Prevent option shadowing.

Review Request #3035 - Created Oct. 24, 2015 and submitted

Information
Benjy Weinberger
pants
4f296a4...
Reviewers
pants-reviews
nhoward_tw, stuhood, zundel

It was unused (at least deliberately, see below...), confusing, and
required some complicated logic to support.

Highlights:

  • Simplifies 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 __getattr__/__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
    -f, --foo, we can now only access the value as opts.foo, not opts.f. But
    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
    following means:

    • 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
      global --fail-fast meant "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 --tag option that
      wasn't necessary: the global --tag option already filters targets
      by tags, and the task had a more general --tag-regex option already.

    • Removing the outer option. Specifically, since so many tasks register
      a --version option, it made no sense for the global scope to hog that
      option name for itself. So now only -V and --pants-version work for
      getting the version of pants itself. Seems reasonable, especially since
      you already had to use pants_version in config, due to some special-casing.

    • Changing task registration names. E.g., the JvmBundle task was
      registered as bundle under goal bundle, meaning that its
      options scope was bundle.bundle, elided to just bundle.
      Which meant that any other task in the bundle goal could shadow its options.
      Renaming the task to bundle.jvm fixes 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 migrate_config.py should
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

Stu Hood
Stu Hood
Benjy Weinberger
Benjy Weinberger
Review request changed

Status: Closed (submitted)

Change Summary:

9c2fc4777623176ef3f41ad20da5addc7ad5e759

Eric Ayers

   

s/compile.zinc/gen.protoc/

This is an option passed to the protoc executable command line. It was be renamed to another option name in [gen.protoc]

[gen.protoc]
protoc_plugins: [ 'foo' ]
  1. Ooops, that was a copy-paste error. Will push a fix.

Eric Ayers

   

Need to add binary.jvm also

  1. I don't follow. Didn't I do that in line 362?

  2. Yep you did. My mistake. The problem I had was in a test, so it wasn't going to be detected by scanning pants.ini anyway.

Loading...