Refactor the deprecation implementation.

Review Request #3682 — Created April 11, 2016 and submitted

benjyw
pants
pants-reviews
jsirois, kwlzn
  • Have all deprecation functions/annotations use the same
    underlying logic to decide whether to warn or error.

  • Previously there was confusion due to inconsistent use of
    "deprecation_version" vs. "removal_version". I standardized
    everything on "removal_version", as that is the less ambiguous term
    ("deprecation_version" could be read by a casual user to mean
    "version at which the deprecation began").
    I also replaced "deprecation_hint" with "removal_hint",
    for consistency with "removal_version".
    Fortunately we currently have no outstanding deprecated options,
    so this was especially easy to do.

  • If removal_version < current pants version, show a sensible error
    about the deprecation to the user, instead of an error (intended
    for pants developers) saying that the removal version must be in
    the future. Previously we had a hack to do that for option
    deprecations, but not for other deprecations. This change makes the
    behavior uniform. It seems far more important to give end users a
    sensible message, than to give pants developers one. It also allows
    us to turn deprecation warnings into errors without actually having
    to remove any code, which makes it easier on releasers: They don't
    have to figure out how to excise the relevant code in order to effect
    the removal. The version change will turn the warnings to errors, and
    the releaser can then ask whoever knows about that code to do the
    actual code change.

CI passes: https://travis-ci.org/pantsbuild/pants/builds/122319746

  • 0
  • 0
  • 2
  • 0
  • 2
Description From Last Updated
BE
BE
  1. Ping?

    1. Before I dive in, this mixes too many concepts fwict. I like consolidating depraction logic. I'm uncomfortable with the semantic changes.

      Kris lays out the proper use here: https://rbcommons.com/s/twitter/r/3369/

      Maybe you can point out here what's wrong with that protocol, quoted below:

      The proposed options deprecation lifecycle is thus:

      1) Options are marked deprecated at release X. From here on, pants throws deprecation warnings to users until release X ships.
      2) Once pants hits release X, the deprecated options begin breaking by throwing exceptions that include deprecation hints. Code that depends on these options can then go away.
      3) At some point well into the future (ideally spanning multiple OSS releases to allow for disparate release cycles from OSS vs private), the deprecated options and hints themselves can then be removed.

    2. To clarify- this is exactly the protocol this change implements. But now it enforces it across all deprecations (options, methods, modules and "conditional"), whereas previously this was special-cased for options.

      Note also that the only difference even in those cases is the nature of the error thrown: previously it was an error implying that the Pants programmer has provided an invalid deprecation version (because it's in the past), now it's an error with deprecation hints for the end user (the same message as the warning, but now it's an error).

  2. 
      
ST
  1. 
      
    1. (sorry for the delayed review... was deep in a yak shave)

  2. src/python/pants/base/deprecated.py (Diff revision 1)
     
     
     

    Your python-fu is stronger than mine, but if someone were to be explicit and pass this as a kwarg, this name would be awkwardly long.

    It should be clear enough from the method name what the method does... maybe it's not in this case?

    From callsites, it looks like this is a "name" or "noun" representing the thing.

  3. src/python/pants/option/parser.py (Diff revision 1)
     
     

    removal_version is in here twice now, which I think gets to the root of what John was saying.

    There are two versions that need to be accounted for here: 1) when it will stop working for end users, 2) when pants devs should remove it from the codebase entirely.

    1. Ah, to clarify further - the original 'removal_version' kwarg mentioned here was never wired up to anything. I evidently forgot to remove it here when I renamed deprecated_version.

      We never really accounted for the two versions you mention. The terms deprecated_version and removal_version were used interchangeably in the code to mean 1). And note that pants devs can remove the code that implements the deprecated thing at version 1). 2) is the version at which pants devs can remove the deprecation hint messaging, and as far as I can tell from the commit message and diffs of 055a36d3de496e8367956ac573041f90a063d44f, that was intended to be "some point well into the future", e.g., "5 OSS releases from 1)" or "two minor releases from 1)" rather than some explicitly named version.

      Now that I think of it, maybe removal_version was intended to explicitly name 2) for human readers (pants devs). But that isn't clear in that commit, and the terminology confusion seems to have caused more harm than good.

      So we have two options:

      A) Keep it simple: there's only one version, which is when the feature stops working and its code (but not its hint message) may be removed. And we remove the error at some known future point after that version, e.g., "5 releases away".

      B) Support two version kwargs, but then we need the terminology to be crystal clear.

      I greatly prefer A).

    2. I'm fine with either. A third option might be to have the version be a tuple of deprecation and removal, so that you can't-not set both values.

    3. That seems like more of a change than I was hoping to make, but we could if others feel strongly about it.

      This change implements A), just to be clear.

    4. There is nothing to enforce the "5 OSS releases from 1)"... do we think that things would still be cleaned up quickly enough if there is no enforcement?

    5. I'm not sure how to enforce, TBH. We can't show end users errors, we need the build itself to fail somehow. In any case, I'd prefer to do that in a future change, since we never enforced this until now either.

    6. We do enforce it now. The release process fails noisily if any options are passed their removal deadline.

    7. ...probably just because the build process fails noisily, and the release builds lots of things. But in any case, the effect is that when you bump the version during the release process, options that are scheduled for removal block the release.

    8. Kris has been on vacation for the past week; if this can hold till he gets back this week, that would be appreciated.

    9. That noisy build failure should still happen, the difference is the error message, which is user-facing instead of pants-dev-facing.

      Almost no functionality has changed in this commit - it effectively just brings non-option deprecation behavior in line with that of option deprecations, and even that change is just in the type and message of the exception thrown.

    10. And yes, it can wait, if Kris can do it within the next few days.

  4. 
      
BE
BE
KW
  1. lgtm! I'm a +1 for the 'A) Keep it simple: there's only one version, which is when the feature stops working and its code (but not its hint message) may be removed. And we remove the error at some known future point after that version, e.g., "5 releases away".' approach.

    given the slight recent churn here in terminology and mechanics, it may also be helpful to solidify this with some backing documentation in the developer guide w/ examples and such to make current state crystal clear to developers if time permits.

  2. src/python/pants/base/deprecated.py (Diff revision 2)
     
     

    assuming the raise of this is unhandled and thus the exception type potentially exposed to end users, perhaps a name that sets more explicit/clearer context would make sense?

    something like DeprecationRemovalError?

    also, should this continue to subclass DeprecationApplicationError?

    1. DeprecationApplicationError is a "syntax" error in the deprecation arguments. E.g., a version string that isn't a valid semver. So I don't think this is that kind of error.

      I'll change the name to CodeRemovedError, and it comes with a full message (the same as the deprecation warning), so it should be pretty clear to end users what's going on.

  3. src/python/pants/base/deprecated.py (Diff revision 2)
     
     

    missing closing :

    1. Fixed.

      By the way, turns out we get this wrong in many, many places.
      As far as I can tell from sphinx examples, it should be

      :raises ErrorType: explanation

      And we typically do

      :raises: ErrorType explanation

    2. what constitutes a "correct" rst docstring is a bit fuzzy afaict, but probably best enforced by a linter of some sort that applies our own preferred spec. my understanding is that the type is optional, so :raises: <arbitrary text that includes the type> and :raises ErrorType: <arbitrary text> are both valid. I've never seen the :raises <type>: convention used when something raises multiple types either - would you just separate those with a comma and a space?

  4. src/python/pants/base/deprecated.py (Diff revision 2)
     
     

    missing closing :

  5. src/python/pants/base/deprecated.py (Diff revision 2)
     
     
     

    could it make sense to make hints non-optional to enforce their use?

    1. Possibly, but let's leave that for another change?

    2. sure, sgtm.

  6. 
      
BE
KW
  1. Ship It!
  2. 
      
BE
BE
BE
Review request changed

Status: Closed (submitted)

Change Summary:

083309d40076138c62597dc27cd438968cbaf503

Loading...