Support options whose values are lists of dicts.

Review Request #3580 — Created March 17, 2016 and submitted

benjyw
pants
pants-reviews
gmalmquist, kwlzn, zundel
Support list-typed options with dict, target and file members.

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

  • 0
  • 0
  • 3
  • 0
  • 3
Description From Last Updated
ST
  1. Thanks!

  2. Should be able to execute the comparison under contrib.go.testprojects.src.thrift.thrifttest.fleem/current now (post #3553)... sorry, didn't think of it over there.

    1. Cool! I'll pull and see if that works.

  3. Should this use https://docs.python.org/2/library/collections.html#collections-abstract-base-classes ?

    1. I'm not sure. I fear the unintended consequences, as I'm not sure which python types might creep in.

      In actual fact, the value will always be a list, so we could even get rid of tuple here. But that might, I suppose, change in the future, so I prefer to be robust.

  4. Best not to sort these probably: order could/should have meaning.

    1. Good point, changed and commented (so readers aren't confused by the fact that we do sort target specs).

  5. 
      
BE
BE
ZU
  1. 
      
  2. src/python/pants/option/options.py (Diff revision 2)
     
     

    Is this going to fail in a strange way if fingerprinting is on and list_option is specified but member_type isn't? It should only affect developers, so maybe just a comment would be in order.

    1. I'll add a comment, but we always assume str if a type isn't specified (as does argparse).

    2. Actually I'm not sure what comment I would even add. This shouldn't fail just from falling back to str, right?

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

    This is also missing tuple.

    1. No - we don't allow lists of tuples (or lists of lists) - too confusing.

    2. We certainly used to, and this breaks us internally. What's the issue with lists of tuples?

    3. Well, we used to support literally anything that would eval, but that wasn't very robust (there was nothing to stop you passing a non-list and blowing up much later, for example).

      Currently we need a way to detect whether the user intends to append to a list or replace it, and we do so by looking at the first character. If it's a '[' or a '(' then we assume replacement, otherwise we assume append. So having lists-or-tuples of lists-or-tuples won't work.

      What's your example of a lists-of-lists option?

    4. Possible solutions:

      1. Change the member_type of that option to something else, such as str (which you then interpret as a tuple in your option-using code).
      2. Special-case member_type=tuple/list and disallow appending in that case (you can still use the +[] syntax to extend instead of replace).
    5. Look at the pom-resolve options at the bottom: https://github.com/foursquare/fsqio/blob/master/pants.ini.

    6. Another major issue with this was the way the error presented: silently, the option switched from being interpreted as a list of tuples to being a list of strings. This made it all the way down into real resolution before it errored, because those tuples are basically just used for direct equality comparison. The comparison always came back false, because they silently switched to being strings--very dangerous.

    7. Hmmm, yeah, that is unfortunate... Sorry about the silent breakage.

      We could issue a warning if member_type is missing, requiring it to be specified? But that might be overkill.

    8. I really don't want to go back to opaque lists though. That has its own set of problems.

  3. 
      
BE
  1. 
      
  2. 
      
BE
Review request changed

Status: Closed (submitted)

Change Summary:

58fd18511bbb4ef67b851fc39366c1280bce05c8

BE
  1. This is in master as 58fd18511bbb4ef67b851fc39366c1280bce05c8. Thanks all!

  2. 
      
Loading...