Add a way to deprecate options with 'deprecated_version' and 'deprecated_hint' kwargs to register()

Review Request #1814 — Created Feb. 21, 2015 and submitted

Eric Ayers
pants
zundel/deprecated_options
1152
66b26f8...
pants-reviews
benjyw, jsirois

Followon to discussion in https://rbcommons.com/s/twitter/r/1799/ about deprecating options.

  • Register the help with a deprecation message
  • When a deprecated argument is invoked, print the deprecation message to stderr
  • Refactored @deprecated to re-use logic for deprecation warning on options.
  • Add a deprecation message to the --color option for SpecsRun

Added automated tests to test_deprecated.py and test_options.py

See CI status by clicking on Bugs link in rbcommons.

Manual testing:

devpants test.specs --color
...

*** Option --color in scope test.specs is deprecated and will be removed in version 0.0.30. Use the --no-colors instead.

devpants test.specs --color

*** Option --no-color in scope test.specs is deprecated and will be removed in version 0.0.30. Use the --no-colors instead.

Also, I temporarily registered a deprecation hint on --colors on the global scope to test it.

  • 0
  • 0
  • 3
  • 0
  • 3
Description From Last Updated
Eric Ayers
  1. Sorry to post this without automated tests, I will add those before submitting. I have to do other things this weekend, but wanted to get some feedback on this idea.

  2. 
      
Benjy Weinberger
  1. I like it!

  2. Grammar nit: help message is missing a noun?

  3. Don't forget to undo this...

  4. 
      
John Sirois
  1. 
      
  2. src/python/pants/base/deprecated.py (Diff revision 1)
     
     
    Funny indent here and on line 58 below.
  3. src/python/pants/base/deprecated.py (Diff revision 1)
     
     
    Its a bit of a leak to be raising deprecated specific exceptions that live in DeprecatedUtil.  Since DeprecatedUtil is just acting as a namespace, why not let this module be that namespace and leave the exception types top-level and the check_semver function top level as well?
    1. I reverted my changes to deprecated.py

  4. src/python/pants/option/options.py (Diff revision 1)
     
     
    The deprecated decorator uses `warnings.warn` to signal the warning.  In that case there is the real benefit of de-duplication which won't be a benefit here, but it does seem like sticking with the warnings mechanism is the way to go.
    1. I tried to use warnings. on the first go round, but it insisted on printing out an irrelevant line of source code. I figured out a way around this by intentionally sabatoging the 'stacklevel' parameter

  5. 
      
Nick Howard (Twitter)
  1. Thanks for this. I think it's good to be able to have deprecation notices on flags. I've got a couple minor comments.

  2. This message seems a little obtuse. Maybe something like: 'We are getting rid of a global color option. See individual tasks for color flags.'

    1. reverted. This was a bogus change I made just as a proof of concept

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

    Could you move this to the parser? I don't think that Options should know about whether flags are deprecated or not.

  4. 
      
Eric Ayers
Nick Howard (Twitter)
  1. Found a typo, other than that, looks good to me!

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

    typo: supress -> suppress

  3. 
      
John Sirois
  1. 
      
  2. src/python/pants/option/parser.py (Diff revision 2)
     
     
    I am not bothered by this but you were for the initial @deprecated impl, so highlighting that this allows for a programmer error to tank pants in the narrow case where we ship a not-installed-by-default task with deprecated options.  An example right now is Checkstyle.
    1. checkstyle does have CI tests and should fail if any Task tests are run that execute register() right?

    2. Sure - but IIRC you're arguments against @deprecated checks boiled down to untested code that we released.
    3. We could add a @deprecated_argument decorator to automatically append these 'deprecated_version' arguments.

      @deprecated_argument('0.0.30', '--foo', 'Use the argument --bar instead of --foo.')
      @classmethod
      def register_options(cls, register) ...
      

      The @deprecated_argument method could wrap the passed 'register' argument so that it would modify kwargs when it is actually invoked.

      If you think that would be a great thing, I could work on that as a follow on. However, I'm not that bothered by it as even the most trivial task test like the ones Benjy just deleted will catch a RemovalSemver error with this scheme... Oh wait, those tests are deleted :-(

    4. I am not bothered by this, I was just confused that you were bothered by this in the past but not now.  All good with me.
  3. 
      
Eric Ayers
Eric Ayers
  1. Is there anything blocking this? I was holding off waiting on a ship it from either John or Benjy.

  2. 
      
John Sirois
  1. Ship It!
  2. 
      
Eric Ayers
Review request changed

Status: Closed (submitted)

Change Summary:

Thanks Nick, John & Benjy. Commit df50472

Loading...