Adding a console task to explain where options came from.

Review Request #2816 — Created Sept. 14, 2015 and submitted

gmalmquist
pants
gmalmquist/explicit-args
2192
ff536b2...
pants-reviews
benjyw, jsirois, zundel
Since pants allows options in config files that haven't been
registered, it's easy to accidentally typo an option and have
pants silently ignore your config.

It can also be tricky to know what options end up getting used when
you might have flags set via multiple config files, through env
variables, and by flags set in wrapper scripts.

This makes it easy to verify if options you set in pants.ini or
elsewhere are actually sticking/not getting overriden by something
else.

It might also be cool to plumb this information through to the
pants server UI at some point.

Added tests to tests/python/pants_test/option/test_options_integration.py and tests/python/pants_test/option/test_options.py.

CI went green here: https://travis-ci.org/gmalmquist/pants/builds/80306892
CI went green here: https://travis-ci.org/gmalmquist/pants/builds/80484138

  • 0
  • 0
  • 8
  • 1
  • 9
Description From Last Updated
GM
BE
  1. This is an awesome feature, thanks for adding it. See comments inline.

    1. Accidental comment?

    2. Yeah, nothing to see here.

  2. What does 'overridden' mean here?

    1. It refers to previous values that were overridden by higher-ranked options. Maybe it would be more appropriately named --show-history.

  3. This many unrelated colors might be more distracting than useful. How about using 'faint', regular and 'bold' versions of a single color to signify env vs. config vs. flag? That implies the level of precedence. Or would that be too indistinguishable?

    1. The vast majority of the values are NONE or HARDCODED, which both render as a pale grey color. The second-most common is CONFIG, which is blue, so in practice it ends up looking like mostly grey with a few pockets of blue here and there.

      ENVIRONMENT variables and cli FLAGs are much less common, and are accordingly displayed as bright red and magenta respectively.

      Contrived setup with screenshot here: http://picpaste.com/thumbs/explain-options-screenshot-Eub36UEk.1442263937.png

    2. I get "no pic here".

  4. I don't love that this is a classmethod.

    How about making the tracker a singleton member of the Options instance?

    1. If the OptionTracker was accessible only through an Options instance it would be a bit tricky because add_record is currently invoked in places where we don't have access to an actual Options instance. A class-level Options.get_tracker() would be easy enough, but I don't know if that actually solves the problem here.

    2. did you mean record_option is currently invoked... ? I think the dependency injection pattern could help. Couldn't you instantiate OptionsTracker elsewhere, and pass it in as a parameter to Parser.init() and Options.init()?

    3. Yes. And yeah, it looks like I can init in in OptionsBootstrapper, then send it to the two instances of Options.create in that class, and from there through ParserHierarchy -> Parser.

  5. src/python/pants/option/ranked_value.py (Diff revision 1)
     
     

    I think you don't need this import?

    1. Nope, relic from code that had a brief existence in my branch.

  6. src/python/pants/option/ranked_value.py (Diff revision 1)
     
     

    Might want to sort in some consistent order? Ideally rank order?

    1. Yeah that might be nice.

  7. src/python/pants/option/ranked_value.py (Diff revision 1)
     
     

    LOL at my previous __eq__ function. Must have been on crack at the time.

    1. Good opportunity to add unit tests for these methods.

    2. This is tested now in the unit test I added to test_options.py.

  8. src/python/pants/option/ranked_value.py (Diff revision 1)
     
     

    This seems more confusing than useful. RankedValue is definitely not a tuple in any meaningful way. The two elements (rank and value) are in different spaces, so this "tuple" isn't homogeneous, so I don't quite see what the value is in being able to iterate over it.

    1. When I was writing the code I added to parser.py, I tried:

      choice = RankedValue.choose(None, env_val, config_val, hardcoded_val, default)
      choice_source, choice_value = choice
      

      Because when I was poking around in the debugger previously to figure out how things hooked together, it looked like RankedValue acted like a tuple since its __repr__ function formatted something that looked like a tuple (eg (HARDCODED, some_value)). It was unintuitive to me that something that looked like a tuple when print()ed could not be treated like a tuple, so I added the __iter__ function to make it behave more like it was presented.

      But, of course it would be trivial to just use choice_source, choice_value = choice.rank, choice.value, so if you think the __iter__ is too weird I'll change it.

    2. Being a java fanboy, I usually prefer using the named access to fields. Tuples are cheap and convenient, but if the work has already been done to split it out into fields, just use that.

    3. Yes, I don't think iteration is a meaningful concept here, repr notwithstanding, so I'd rather not support it in the first place.

  9. 
      
ZU
  1. 
      
  2. Maybe it would be better to call this --name?

    I can imagine someone wanting to pass a regular expression or at least a substring to match multiple options. Do you think that would be useful? Maybe that is not necessary - you could just dump them all and use grep to find the ones you wanted.

    1. Yeah that's a better name (no pun intended). It might be nice/reasonable to interpret anything matching ^[/].*?[/]$ as a regex, but I wouldn't want to regex by-default because it could be a nuissance.

  3. nit: this filter is structured just a bit differently than the other two and confused me a bit at first. I wasn't sure if 'True' meant to accept the option or to filter it out.

    1. I can restructure it.

  4. src/python/pants/base/config.py (Diff revision 1)
     
     

    If this is meant to be overridden by a subclass, document it as such, or decorate it with @abstractmethod (and mixin the AbstractClass support)

    1. Okay

    2. Other places in the class just doc and raise NotImplementedError, and changing to ABCMeta seems to cause all sorts of problems, so I'll just do that.

  5. I previously commented that you should add tests, but I let that comment fly early and it looks like you did right here.

  6. It looks a bit fragile that this test depends on some completely unrelated task (gen.wire) and the assumption that we won't be setting a value for it in pants.ini. At the very least, it deserves a note, but is there something more intrinsic to pants we could substitute for the wire generator? Maybe publish.jar or something like that?

    1. Yeah I can use a different option; I just used gen.wire because it was on my mind.

  7. 
      
JS
  1. Since you've got 2 strong sets of eyes on this one I'll step away.
  2. 
      
GM
ZU
  1. 
      
  2. src/python/pants/option/options.py (Diff revision 2)
     
     

    document option_tracker. Looks like it is a mandatory argument now.

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

    bug: should be raise cls.OptionTrackerRequiredError() (add simple unit test?)

  4. 
      
GM
ZU
  1. Ship It!
  2. 
      
BE
  1. Ship It!
  2. 
      
GM
Review request changed

Status: Closed (submitted)

Change Summary:

In commit ce4a69c00367297706252f1429b11d25f505a2b9, thanks Eric & Benjy!

Loading...