Support options with type=bool.

Review Request #3623 — Created March 29, 2016 and submitted

benjyw
pants
3626
pants-reviews
kwlzn, mateor, stuhood, zundel

This will ultimately replace action='store_{true|false}'.

In this new implementation, boolean flags work like any other type.
In particular, you can pass an explicit boolean-valued literal:
--foo=true, --bar=False etc.

Support for boolean flags with no value (--foo --no-bar etc.) is now
implemented via the existing implicit_value mechanism. As you may recall,
implicit_value is a registration arg, available on any option type, that
provides a value to be used if the flag is specified with no value.
This allows us to have less special-casing of boolean flags.

All existing old-style boolean options have been munged into
new style ones, so they are effectively implemented via this
new implementation. Therefore deprecating them in the near future
should be straightforward.

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

  1. Thanks Benjy! Nice improvement.

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

    Should be private?

    1. Yeah, probably... Done.

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

    Should the conditional deprecation live here? Or maybe these could refer to constants for store_false and store_true with a comment about deleting them after the deprecation cycle?

    1. Not sure I understand the question. Nothing has been deprecated in this change. That will be in a followup. This just maps the old-style boolean registration onto the new one, behind the scenes, so that we don't have two implementations even in the interim.

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

    Unclear why this code needs to be added now... implicit_value existed before, so is this just fixing a bug where implicit value wasn't being applied before?

    1. implicit_value didn't previously exist for boolean options, because it would have made no sense. implicit_value was a way to make non-boolean options take some value if specified with no value. I can't remember why, but we needed that for some reason. So when I came to implement type=bool, leveraging implicit_value seemed like a natural fit. The special casing of type=bool in this file (e.g., these lines) is needed because type=bool is special in three ways: A) we don't allow None as a value, B) we always want these to have implicit_value behavior (even if the registration didn't provide an implicit_value), and C) we generate the --no-foo inverse flag.

  5. 
      
  1. Ship It!
  2. 
      
Review request changed

Status: Closed (submitted)

Change Summary:

319d55448cf4f3bd86fc56987a495f2e2858dc62

  1. Submitted as 319d55448cf4f3bd86fc56987a495f2e2858dc62. Thanks Stu and Mateo.

  2. 
      
Loading...