Reimplement list options to support appending.
Review Request #3541 - Created March 7, 2016 and submitted
|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:
To replace the existing value, just set to a list:
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
In order to support lists of values other than strings, this change
introduces a member_type registration arg, which is only valid when
This change was more complex than you'd think, as it required refactoring
Parser._compute_value. That method is now clearer and better documented
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
CI passes: https://travis-ci.org/pantsbuild/pants/builds/114384803
Thanks Benjy! Great cleanup.
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.
Worth continuing to include
DEFAULThere for a while, with a conditional deprecation?
As a thought experiment: would this be cleaner if this was a check
if kwargs('type').is_mergeable, with the implementation of
mergeliving elsewhere (in a typeclass) or on the type?
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.
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?
Looks like a fix unrelated to the append work? Its going to get lost in the changelog.
Update description to include callouts to release note items.
Ooops, forgot to upload the code review addressing diff earlier...