Reimplement list options to support appending.

Review Request #3541 — Created March 7, 2016 and submitted

benjyw
pants
pants-reviews
jsirois, patricklaw, stuhood, zundel

[To releasers: See end of this message for things to call out in release notes]

To append a single value to a list-valued option, just "set" the option
to that value, as with action='append'.

./pants --foo=1 --foo=2

To append several values, add a plus sign in front of the value:

./pants --foo=+\[1,2\]

To replace the existing value, just set to a list:

./pants --foo=\[1,2\]

Note that this syntax works with config and env values as well. This
means that you can append to the value specified in, say, pants.ini,
but also override it entirely, if you need to.

This endows list_option with the only useful feature of action='append', which
will therefore soon be deprecated. In fact, action='append' is now implemented
as a list_option under the covers, so replacing all uses of it should be
straightforward.

In order to support lists of values other than strings, this change
introduces a member_type registration arg, which is only valid when
type=list_option.

This change was more complex than you'd think, as it required refactoring
Parser._compute_value. That method is now clearer and better documented
than before.

Some behavior has changed slightly. E.g., previously we validated
values even if they weren't used (e.g., the config, when it was overridden
by a cmd-line flag). Now we only validate the actual final value. But
these changes are either positive or neutral, in terms of their impact,
and they make the code more straightforward.

Release notes should also call out the following:

  • This commit tightens up some aspects of option type conversion. There may be options
    in plugins that were relying on broken behavior (such as when using a string where an
    int was expected), and that will now (correctly) break.
  • This commit deprecates the PANTS_DEFAULT_* env vars in favor of PANTS_GLOBAL_*.

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

  • 0
  • 0
  • 1
  • 0
  • 1
Description From Last Updated
  1. Thanks Benjy! Great cleanup.

    1. Oh, one more thing: shpelling in review title.

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

    Just + I think.

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

    It looks like this will always represent a fragment of the final value that is exposed. Might be worthwhile to indicate that it is partial in the name. ListPartialValue, ListValueFragment, etc. Also, this doc doesn't make that clear.

    1. Changed name to ListValueComponent, and added a lot more comments and docstrings.

  4. Worth continuing to include DEFAULT here for a while, with a conditional deprecation?

    1. I only added this option a few days ago. I am very confident that no one is setting that env var...

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

    As a thought experiment: would this be cleaner if this was a check if kwargs('type').is_mergeable, with the implementation of merge living elsewhere (in a typeclass) or on the type?

    1. I was wondering about that myself, because in the future we might want some similer mergeability for dict_option. 
      
      But that's a bigger change, as most of kwargs('type') are str, int, float or other built-in types that we can't augment with an is_mergeable, so we'd have to do some wrapping under the covers. I'd prefer to do this in a separate change TBH.
    2. Fine with me!

  6. 
      
  1. 
      
  2. Is this just for clarity or is there something that now breaks if you don't declare the type as int? I'm just wondering if something is going to break in plugins that we need to call out in release notes.

    1. This was always broken. It somehow didn't trigger an error before, not sure why. But yes, we might want to call this out in release notes. Will add to the commit message.

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

    Not I only see 2 references to target_list_option,

    scala_js_platform.py and options_fingerprinter.py

    is it that hard to change them now or do you plan a followon soon?

    1. I haven't tried, so I don't know how hard it is. But I don't want to deal with it here - this change was already hairy enough...

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

    Looks like a fix unrelated to the append work? Its going to get lost in the changelog.

    1. I'll call it out explicitly in the commit message, so it ends up in the release notes.

  5. 
      
  1. Ship It!
  2. 
      
  1. Submitted as 7f1403ef9c8289719620cc44b33a85deb232f75e. Thanks for the reviews and the earlier help brainstorming this!

  2. 
      
Review request changed

Status: Closed (submitted)

Change Summary:

7f1403ef9c8289719620cc44b33a85deb232f75e

Loading...