Some small fixes to the new options implementation.

Review Request #1236 — Created Oct. 28, 2014 and submitted

benjyw
pants
a78f30d...
pants-reviews
johanoskarsson, patricklaw, zundel

These all relate to options whose action is 'append', and address rare corner cases.

  1. Switch some invocations of copy() to deepcopy(), to ensure e.g., that
    default values of list type are not shared between the
    parser we use to generate help and the parser we use to actually
    parse args.

  2. Distinguish between an empty list value in config, and no value at all.
    This ensures that "no value" doesn't trump a hard-coded default because
    we mistake it for an explicit config value.

  3. Don't special-case appends to RankedValue. Instead make copy()
    return the underlying value. argparse is the only thing that calls copy()
    on a RankedValue, and this will ensure that argparse always works with
    raw values, both in the list case and (as before) in the single value case.

  4. Nits: Remove an unused import, break up an overlong line.

Various manual tests of corner cases that could trigger these issues. CI running.

IT
  1. lgtm!

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

    Just to repeat in my own words: before, argparse made a copy of the RankedValue instance using the default implementation of copy, and then called 'append()' on the copied element which was a RankedValue. It isn't really a list, so we hacked in the append() method.

    Now, when argparse calls copy() it gets a copy of the list back instead, so when it calls append() it is holding a native list, and using the append() from it.

    1. That is exactly right. This is slight abuse of __copy__, but the little bit of code that expects a RankedValue can also receive a regular value (it assumes it's at rank FLAG, set by argparse), and that was already the case for non-append options anyway, so if anything this is more uniform than special-casing append.

  3. 
      
ZU
  1. Ship It!

  2. 
      
BE
Review request changed

Status: Closed (submitted)

Change Summary:

Submitted as b71650905e11ae9be48344af951122af4c642942.
Loading...