Fix a bug in OptionTracker.

Review Request #3643 — Created April 2, 2016 and discarded

benjyw
pants
pants-reviews
gmalmquist
Previously, if an option set in config (for example) had no hardcoded
default then the option tracker wouldn't see it as overridden.

This commit modifies the integration test to expose this problem,
and fixes it.

That said, I assume the previous logic was there for a reason, so
if this change is naive or otherwise wrong - please point it out!

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

  • 1
  • 0
  • 0
  • 0
  • 1
Description From Last Updated
I don't think this is what I had intended, though the comment is misleading. If self.latest.rank > RankedValue.HARDCODED was sufficient, ... GM gmalmquist
BE
GM
  1. =

  2. I don't think this is what I had intended, though the comment is misleading. If self.latest.rank > RankedValue.HARDCODED was sufficient, the --only-overridden option wouldn't be necessary at all, because --rank=HARDCODED can do that.

    I believe I explicitly intended options that overrode "None" to not count as overriding, because they're not really overriding anything -- they're the first value the option was set to. The purpose of --only-overridden is to see what options have values that are different from other values they are set to; a major use of that is to detect when options are accidentally inconsistent (ie when something is coming from both an environment variable and an ini file, or both an ini file and the CLI).

    So, I think there is a bug here, but it's in the documentation, not the code.

    1. I see, although it seems a little odd. What about options with an implicit_value (which will soon be all boolean options)? What's the expected behavior there?

    2. I think that's still in the same spirit of NONE; they are still zero-values, right?

    3. Not necessarily, they can be anything. But actually I guess that's not a problem, since there's still an explicit flag.

      The underlying problem I'm having is that boolean options can't take None as a value now (well, after my next change). It was unintentional that they could in the past (they can't in argparse, for example), and preserving that weird three-state logic is a bad idea.

      So I'm not sure what to do here wrt boolean options.

    4. I think False is a reasonable default that would preserve existing behavior?

    5. It's a bit more complicated than that, but from running this through a debugger to see what triggers, I think my understanding of the problem is backwards.

  3. 
      
BE
  1. OK, looks like I can discard this. But it would be great if you could update that comment. Thanks!

  2. 
      
BE
Review request changed

Status: Discarded

Loading...