Prevent option shadowing.
Review Request #3035 - Created Oct. 24, 2015 and submitted
|nhoward_tw, stuhood, zundel|
It was unused (at least deliberately, see below...), confusing, and
required some complicated logic to support.
OptionValueContainer. We still need some magic to support
ranked values, but we no longer need option forwarding magic, which
was a big part of the complication in that file. Now, instead of its
own logic being largely (and complicatedly) implemented in terms of
hasattr/setattr/getattr, it simply wraps a key->rankedvalue dict,
and there's a small wrapper of
__setattr__magic, just to
support direct attribute access.
Also removes the ability to access an option's value by any of its registered
aliases. This wasn't used in practice, and complicated the code needlessly.
So for example, in the rare case where we register an option with two names, say
--foo, we can now only access the value as
we have almost no such options anyway, so this is hardly important. Now you
must access the option by its 'dest' name, as determined by argparse, namely
the first name in the registration list with len(name) > 1.
We now detect attempts to shadow options, and error out on them.
Unfortunately, we had quite a few unintentional shadowed options in
the codebase (and some of these were quite confusing, so it's a good
thing we got rid of them). Each such case was fixed by one of the
Renaming the option.
Deferring to the global option of the same name (in some cases
making that option recursive). This meant expanding the meaning of
the global option, but in a sensible way. E.g., previously the
--fail-fastmeant "fail fast on parsing", but now it means
"fail fast in whatever way makes sense to the code using the option".
Removing the inner option. E.g., the filter task had a
wasn't necessary: the global
--tagoption already filters targets
by tags, and the task had a more general
Removing the outer option. Specifically, since so many tasks register
--versionoption, it made no sense for the global scope to hog that
option name for itself. So now only
getting the version of pants itself. Seems reasonable, especially since
you already had to use
pants_versionin config, due to some special-casing.
Changing task registration names. E.g., the
bundle, meaning that its
options scope was
bundle.bundle, elided to just
Which meant that any other task in the
bundlegoal could shadow its options.
Renaming the task to
bundle.jvmfixes this issue, and is also
a good idea anyway. There's no reason to privilege JVM in this way.
Note that unfortunately it was not feasible to do a deprecation cycle
on these option changes, as they interact with the code in subtle ways,
and getting deprecation right would have been a lot of work.
However they were mostly config-only options, and
deal with them. When this rolls out, repo owners are going to have
to pay extra attention to potential issues in their repos, but that
was going to be true anyway, no matter how this change shaped up.
CI passes: https://travis-ci.org/pantsbuild/pants/builds/87215317
Allowing tasks to use
versionprobably makes sense, but I think that it would be good to preserve the ability to run
./pants --version, one way or another. It's always a bit annoying when none of
-vdo anything for a unix process.
Could we special case "exactly one argument to pants and it is
Should we invert this and rename the global
Since it is loaded from the
[DEFAULT]section of pants.ini, the current name is already somewhat ambiguous.
Which makes me wonder... what would happen if all of the global options were instead in a
pants-scoped global subsystem?
Then, with a very small special case to scope the options that immediately follow
./pantson the command line to the
pantssubsystem, you'd get working positional/short options passed to that subsystem, without ambiguity about the
Also, it would immediately suck all of the global options out of the
[DEFAULT]section and into a
[pants]section, which I think we've wanted for a while.
Revision 2 (+224 -383)
Status: Closed (submitted)