support mutually_exclusive_group paramater in option registration

Review Request #4336 — Created Oct. 27, 2016 and submitted

yujiec
pants
3881, 4002
pants-reviews
benjyw, mateor, stuhood

The idea originates from option renaming process. To rename an option, we have to write a custom logic to prevent both options from being given and route value from "dest" of old option to "dest" of new option. With a "mutually_exclusive_group" argument in register(), old option can easily set it to be the new option. This patch implements the general "mutually exclusive" logic for options.

The way mutually_exclusive_group works is, in register() method, a user can specify "mutually_exclusive_group=XXX", then "XXX" will be the dest for this option. "XXX" can be the name of another option, or a totally new name. When more than 1 option with dest being XXX given through flags, env var or config file, pants will throw an exception.

https://travis-ci.org/pantsbuild/pants/builds/171997616

  • 0
  • 0
  • 1
  • 0
  • 1
Description From Last Updated
  1. This is a neat idea! I have a question and a terminology nit.

    As far as I can tell this has nothing directly to do with renaming options, right? It can be used when renaming options, but you still need to implement the logic that reads from one or the other option?

    Regarding terminology: mutual_exclusive isn't a grammatically coherent name. How about exclusivity_label or something like that? We want to be clear that this is some logical name for an equivalence class of options. I don't know that the word "mutual" adds any information here.

    1. For the question:
      You are right. It's just I have this idea of a deprecated_to field when renaming an option to save work during option parsing (dest routing, exclusive check). Then Stu suggests I can implement the general mutually exclusive logic. The complete renaming process requires more work than just setting this new field.

      For terminology:
      Python argparse module has add_mutually_exclusive_group() method. But that is a bit long and I think "exclusivity_label" sounds good.

    2. I didn't know argparse had something similar, in that case I'd be in favor of using the same name, for uniformity, i.e., mutually_exclusive_group=, despite the clunkiness.

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

    Phrasing nit: """Raised when more than one option belonging to the same mutually exclusive group is specified."""

  3. 
      
  1. 
      
  2. Off topic, but is this the list of default ignore_patterns? It would probably be good to throw .pants.d and .git in here as well, at minimum, and maybe move into a constant at the top of the file.

    Sorry for OT, just struck me as I was reading.

    1. '.*' already includes '.pants.d' and '.git' dirs. I think this default list is only used here (ignore-patterns will be deprecated soon, thus only use case is build-ignore), and all the other options have their default values in register() methods, thus I prefer to leave it as it is now just to follow the convention.

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

    Can you subclass MemberTypeNotAllowed or something else from pants.options.errors? I think catching OptionsError should also catch these.

    1. I will subclass ParseError since it is thrown during parsing stage. Actually I wanted to do it but forgot to change it when I copied from goal_runner.py. Thanks for catching this!

  4. 
      
Review request changed

Status: Closed (submitted)

Change Summary:

Merged in https://github.com/pantsbuild/pants/commit/ba4269f3b1cfc0a414efde2aad8337f1251550b2.
Thanks Benjy and Mateo!

Loading...