Disallow --no- on the name of boolean flags, refactor existing ones

Review Request #1799 — Created Feb. 19, 2015 and submitted

zundel
pants
zundel/fix-no-colors
1133
444940c...
pants-reviews
benjyw, fkorotkov, jsirois

This came out of an issue with the IntelliJ plugin. See https://github.com/pantsbuild/intellij-pants-plugin/issues/34

  • Rename --no-colors to --colors and invert the logic that checks it.
  • --no-locks does not seem to be implemented anywhere. Removed this flag.
  • Fixed up removing color output for java compilation.
  • Added a deprecation message for --color in specs_run
  • Passing --no-color to running python tests seems to work - I think this option is just passed through to pytest but I was unable to find the exact mechanism on how this works.

devpants compile examples/src/java/com/pants/examples/hello/main --no-colors

Note that ./pants clean-all --no-colors does not work, but ./pants --no-colors clean-all does.

BE
  1. I think this isn't the full diff.

  2. While here, can you remove the "Defaults to True" from the help message? We already know the defaults, there's no need to mention them manually.

    1. Sorry about that. Fixed that and pushed again with the remainder of my change.

  3. 
      
ZU
ZU
  1. 
      
  2. I tried adding a property decorated with @deprecated, but I cannot get this message to print. I thought it would be invoked by ./pants test but it doesn't seem to be.

    1. We have 0 targets to test this with - I'm sure the @deprecated property works:
      ```console
      $ find . -name "BUILD*" | xargs grep "scala_tests|scala_specs"
      $
      ```
    2. Its not that I'm not sure that @deprecated works, its that we won't catch it when we upgrade to 0.0.30 if we never invoke it, will we?

      The option error message specifically mentions functions, and only gets invoked when the function is invoked. I was thinking that
      we really want to @deprecate the call to register the option, but I don't think you can use a decorator at a call site.

      What do you think about a different way to deprecate options, like:

      register('--crufty', deprecated_version='0.0.30', deprecated_message='Use --replacement instead.',
      ...)

      Then, we can do our deprecation checks when the call is registered and throw if the current version is past the deprecated version,
      always show the deprecated message in help, and when the option is actually used or set in pants.ini, spit something out to stderr.

    3. Sorry, my comment is confusing, what I meant to say is,

      "The @deprecated error message specifically mentions functions,..."

  3. 
      
BE
  1. Awesome, thanks for cleaning this up.

  2. Thanks for fixing this!

  3. 
      
JS
  1. Mod a @deprecated property really should be used.
  2. 
      
ZU
ZU
ZU
Review request changed

Status: Closed (submitted)

Change Summary:

Thanks for the reviews. Submitted at 6453c0f

Loading...