Add an option for indicating a direct replacement of a deprecated variable.

[jmalloc]

old := ferrite.
    Duration("OLD", "example duration variable").
    Deprecated(
        WithDirectReplacement(new),
    )

new := ferrite.
    Duration("NEW", "example duration variable").
    Deprecated()

The goal here is twofold:

1. Allow documentation to point to the replacement environment variable
2. Surface the value of the new variable via the old.DeprecatedValue() method

Resolution logic for old.DeprecatedValue() pseudo code

if OLD is defined {
  if OLD matches schema of OLD {
    return OLD
  }
  return error
}

if NEW is defined {
  if NEW matches schema of OLD {
    return NEW
  }

  // we _could_ optionally return nil here indicating no value is available,
  // but I think that would be less useful
  return error 
}

return nil

The behavior of NEW is unaffected, it is unaware that it is the replacement for OLD.

[jmalloc]

/cc @ezzatron I would appreciate your thoughts on this. Note this is specifically about the new WithDirectReplacement() suggestion, the Deprecated() feature is already on main.

The .DeprecatedValue() naming might be a little confusing. This naming is to differentiate it from the .Value() method of NON-deprecated variables so that when you deprecate a variable you are forced to identify the places within your codebase where it is used and explicitly opt-in to continue using the deprecated variable if you wish to do so.

[ezzatron]

I guess that way could work. .DeprecatedValue() is kind of backwards to how I would have attempted to implement it myself though. I'd want to be able to update my consuming code to use the new variable immediately, and simply delete the old variable when ready without any changes to consuming code (or as few as possible). That would lessen the burden a few months down the track when it's time to remove your deprecated value.

So I suppose that might make the regular .Value() pseudocode look more like:

if self is defined {
  if self is valid {
    return self value
  }

  return error
}

if self is replacing x {
  if x is valid {
    return x value
  }

  return error 
}

return nil

In this approach you might start with this:

old := ferrite.Duration("OLD", "example duration variable")

func main() {
    fmt.Println(old.Value())
}

Do the majority of the work to set up and use the new variable in one change:

  old := ferrite.Duration("OLD", "example duration variable")
+     .Deprecated(
+         WithDirectReplacement(new),
+     )
+ 
+ new := ferrite.Duration("NEW", "example duration variable")
  
  func main() {
-     fmt.Println(old.Value())
+     fmt.Println(new.Value())
  }

Then future you doesn't have much mental overhead when it's time to remove the deprecated variable:

- old := ferrite.
-     Duration("OLD", "example duration variable").
-     Deprecated(
-         WithDirectReplacement(new),
-     )
- 
  new := ferrite.Duration("NEW", "example duration variable")
  
  func main() {
      fmt.Println(new.Value())
  }

It probably also depends what you consider a "direct replacement". I would only expect Ferrite to manage all of this for me if the type of values being produced were identical. If the new variable doesn't produce the same type but is still conceptually a replacement, I might want some way to indicate that so that it's documented, but I don't think I'd expect Ferrite to handle the fallback logic for me, and this might be a case where adding some free-form notes to a variable would suffice.

[jmalloc]

Thanks man! ❤️ Wall of text incoming, apologies! We can chat about this in person, but I'll still get my thoughts down here.

I think the key take away here is that rather than surfacing the NEW value via the old variable, you'd have new fall-back to the value of OLD. Is that right? If so I think that does make much more sense!

Perhaps the deprecated variable could also look at the value of NEW, making it a bidirectional relationship.

It probably also depends what you consider a "direct replacement". I would only expect Ferrite to manage all of this for me if the type of values being produced were identical.

I don't think it matters whether the replacement is the same type, as ultimately the input is always a string. Consider for example, migrating from a String() variable type, to the more recently added URL() type. Assuming there's some reason you're not just changing the type of the existing variable, a syntactically valid URL supplied as a value to the original String() variable will still be valid when evaluated as a URL().

So I'd define a direct replacement as:

1. Having a different name, obviously
2. Having the same semantic meaning to the user
3. Negating the need for the deprecated variable

So perhaps logic as follows:

if self is defined {
  if self is valid {
    return self value
  }

  return error
}

if self is replacing x {
  if x is defined {
    if x is valid as per self's definition {
      return x value
    }
  }
}

if self is required {
  return error
}

return nil

In summary, fallback to OLD only if it's valid according to NEW's schema, otherwise simply ignore it.

this might be a case where adding some free-form notes to a variable would suffice

Yep, definitely want to be able to do this. Was thinking also of allowing a SeeAlso() option (independent of deprecation features).

---

The rest of this reply is just me trying to get my thoughts down about the .DeprecatedValue() decision.

I'd want to be able to update my consuming code to use the new variable immediately, and simply delete the old variable when ready without any changes to consuming code (or as few as possible)
...
Then future you doesn't have much mental overhead when it's time to remove the deprecated variable:

This is definitely the intent. Ferrite using the .DeprecatedValue() naming forces you to update your consuming code immediately or otherwise deliberately opt-in to continued use of the deprecated variable. I wouldn't want to disallow use of the deprecated variable if using the new one is difficult in the short term or happening as part of a larger refactor, for example.

The .Deprecated() method is an alternative to the existing .Required() or .Optional() methods; it cannot be used in conjunction with them. This follows from my assumption that if you're deprecating a variable you want people to stop using it and therefore it's nonsensical to have a variable that is both required and deprecated.

With that in mind, a change from .Required() to .Deprecated() would cause a change to the return values of .Value() anyway, as optional variables return a boolean indicating whether a value is available. Using a different name for this method ensures that the existing consuming code breaks in the same way regardless of whether the variable was previously required or optional.

Anyway, this makes more sense with the pseudo code you suggested, as best I can tell :)

[ezzatron]

I think the key take away here is that rather than surfacing the NEW value via the old variable, you'd have new fall-back to the value of OLD. Is that right? If so I think that does make much more sense!

Yep, that was my main thought.

I don't think it matters whether the replacement is the same type, as ultimately the input is always a string. Consider for example, migrating from a String() variable type, to the more recently added URL() type. Assuming there's some reason you're not just changing the type of the existing variable, a syntactically valid URL supplied as a value to the original String() variable will still be valid when evaluated as a URL().

Yeah, okay. I can see why that would be desirable.

In summary, fallback to OLD only if it's valid according to NEW's schema, otherwise simply ignore it.

An issue I see with this logic is in the case where:

• NEW is required
• NEW's value is undefined
• OLD's value is defined
• OLD's value is invalid according to NEW's schema

The user is going to see a message saying that NEW is undefined, with no indication that the problem is actually because OLD's value doesn't match NEW's schema. This means there's a potential for BC breaks from the user's perspective, which are easy to overlook from the implementor's perspective. Ideally you want any BC breaks to happen when you finally remove the deprecated variable.

You could work around this as an implementor by making them both optional and validating that one is set correctly in consuming code, but that kind of undermines the goals of making it easy to remove deprecated stuff later.

I think I'd prefer, when falling back to the deprecated variable, that if that variable doesn't match the new schema, it produced an error indicating as much. Now, I guess that's still a BC break technically, but you could argue in cases like string to URL transition, it was always supposed to be a valid URL, so if you were able to supply a poorly-formatted URL string before, that was a bug, not a feature. At least the user gets an error message that clearly explains what's actually happening.

[ezzatron]

In other words, I'm just advocating for this change:

  if self is defined {
    if self is valid {
      return self value
    }  

    return error
  }  

  if self is replacing x {
    if x is defined {
      if x is valid as per self's definition {
        return x value
      }  
+
+     return error
    }
  }  

  if self is required {
    return error
  }  

  return nil

[jmalloc]

The user is going to see a message saying that NEW is undefined, with no indication that the problem is actually because OLD's value doesn't match NEW's schema.

This doesn't have to be the case, the information is available to explain exactly what happened.

Ideally you want any BC breaks to happen when you finally remove the deprecated variable.

I like this, I think this has to be one of the main design considerations of Deprecated() in general 👍

I think I'd prefer, when falling back to the deprecated variable, that if that variable doesn't match the new schema, it produced an error indicating as much.

Yep, that makes sense. Otherwise, for optional variables at least, it would introduce the first case where a provided value is ignored completely.

You could work around this as an implementor by making them both optional and validating that one is set correctly in consuming code, but that kind of undermines the goals of making it easy to remove deprecated stuff later.

For sure. This is the exact thing I want to avoid, because it's not introspectable, and therefore I can't explain it to the user automatically.

[jmalloc]

I've added a SupersededBy() deprecation option. It doesn't yet perform any of the fallback logic discussed above, but successfully models the relationship between the two (or more) variables, and includes some information in the generated documentation.

[jmalloc]

Moving out of v1.0.0 milestone. As discussed this particular approach adds a situation where a variable spec is modified after it has already been "finalized" (and perhaps already used by the application in uncommon cases).

I would still like to explore other options for achieving the same thing here, but we don't have a pressing need ourselves anymore.