Adds custom list and dict option types.

Review Request #1389 — Created Nov. 22, 2014 and submitted

benjyw
pants
8f217e6...
pants-reviews
ity, patricklaw, zundel

Values (in cmd-line args, env vars and config files) must be JSON lists/objects.

Currently our config reader 'parses' list and dict values using python 'eval', so we
change all of those in our pants.ini to be valid JSON. Fortunately they remain valid
eval-able python too. Eventually we'll get rid of that eval-ing code, but for now
we need to support both, during the transition.

Adds checks in migrate_config.py for list/dict values that aren't valid JSON.

Note that the list type is different from action='append'. The latter
appends cmd-line arg values to the default. The former replaces the
default.

CI passes.

  • 0
  • 0
  • 1
  • 0
  • 1
Description From Last Updated
ZU
  1. 
      
    1. Thanks for the review! I've addressed your comments. Please take another look when you can.

  2. pants.ini (Diff revision 1)
     
     

    Add a note that values must use JSON format.

  3. pants.ini (Diff revision 1)
     
     

    This file is not present in the repo. Remove this setting from pants.ini?

    1. I feel like changes to Pants's config system, which imply changes to all pants.ini files in the world, are in a different category than changes to Pants's own pants.ini file. So I'd rather not mix the two too much.

      For example, in this case checkstyle.py would be broken if that key wasn't present (even though it's also currently broken in a different way because that file doesn't exist.) So a better solution might be for someone who knows/cares about checkstyle to provide a coding_style.xml, whatever that is... Or for us to remove it from core pants. Either way, it's a different set of considerations than those relevant to this change.

      When all these config changes are done I definitely want to audit our pants.ini and get rid of a bunch of things. But for now I prefer to make that orthogonal to this effort.

    2. Filed issue https://github.com/pantsbuild/pants/issues/825

  4. pants.ini (Diff revision 1)
     
     

    This file is also not present in the repo.

  5. pants.ini (Diff revision 1)
     
     

    Wow, I never saw that you could inlclude arbitrary python in the .ini file before. I can see how it was handy, but I looked around and said that everyone says, "Use JSON" for this kind of thing.

    1. This one was surprising to me too...

  6. pants.ini (Diff revision 1)
     
     

    why %(buildroot) and not $(pants_supportdir)?

    1. No idea. Again - would prefer to fix such things in some other commit.

  7. src/python/pants/option/custom_types.py (Diff revision 1)
     
     

    I put single quotes around a key gen for a dict

    thrift-gen.java: {
     'gen': "java:hashcode",
     "deps": {
       "service": ["3rdparty:thrift-0.5.0-finagle"],
       "structs": ["3rdparty:thrift-0.5.0"]
     }
    }
    

    devpants goal gen.thrift --lang=java ./examples/src//thrift/com/pants/examples/distance:distance-java

    It worked? I guess this is still going through the python eval code.

    1. Correct, this is still being eval'd. I will of course get rid of that code, but wanted to keep this change reasonable, and do that later.

  8. src/python/pants/option/custom_types.py (Diff revision 1)
     
     

    Here is the error message we get:

    10:21:17 00:00   [bootstrap]
    10:21:18 00:01   [setup]
    10:21:18 00:01     [parse]
                   FAILURE
    
    Exception message: No valid list for java-compile.jvm_args: ['-Xmx2G"]
    EOL while scanning string literal (<string>, line 1)
    

    It would be nice if it mentioned 'pants.ini' somewhere and that the expected syntax is JSON format. It would be even better if it would print the line number of pants.ini.

    1. Yeah, that's not a super-helpful error message... However this code knows nothing about pants.ini. The error could equally be in the format of a command-line arg or an env var. I've improved the error message as best I can without re-jiggering stuff inside the options system though.

  9. 
      
BE
ZU
  1. 
      
  2. src/python/pants/option/custom_types.py (Diff revisions 1 - 2)
     
     

    nit: pep8: use camel case

    https://www.python.org/dev/peps/pep-0008#exception-names
    Because exceptions should be classes, the class naming convention applies here. However, you should use the suffix "Error" on your exception names (if the exception actually is an error).

    1. This is just a convenience method that returns a ParseError instance. It's not itself a class, let alone an exception type. So I think the method_name_convention applies. I've added a docstring to make its role clearer.

  3. 
      
NH
  1. 
      
  2. Should this be DEFAULT?

  3. 
      
BE
BE
Review request changed

Status: Closed (submitted)

Change Summary:

Submitted as 8c3b5b33e8cabdf599ed1332067283ffc067a13a.
PA
  1. Ship It!

  2. 
      
Loading...