Provide more specific value parsing errors

Review Request #2283 — Created May 28, 2015 and submitted

jrosenfield
pants
jessrosenfield/parsing
1249, 1604
332d8c8...
pants-reviews
areitz, benjyw, jsirois, zundel
Provide more specific value parsing errors

Travis CI build green https://travis-ci.org/pantsbuild/pants/builds/64502793

~/Src/pants ./pants publish examples/src/thrift/org/pantsbuild/example/distance:distance-java --publish-jar-restrict-push-branches='foo/bar'

throws

Exception message: Error while parsing option value foo/bar: Value cannot be evaluated as an expression: name 'foo' is not defined
1: foo/bar
Acceptable types: list, tuple

instead of

Exception message: Error while parsing option value foo/bar: Value cannot be evaluated: name 'foo' is not defined
1: foo/bar

as a resolution to http://github.com/pantsbuild/pants/issues#issue/1249

  • 0
  • 0
  • 1
  • 0
  • 1
Description From Last Updated
JS
  1. 
      
  2. src/python/pants/option/custom_types.py (Diff revision 1)
     
     

    :returns: ... would be good while you're at it

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

    Not yours but s/e.message/e/ - the message field of exceptions is deprecated - just __str__ of e does what we want.

  4. 
      
ZU
  1. Could you update the 'testing done' section with the command line for the manual reproduction of the bug that Andy reported. Also include what the new output looks like.

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

    get_types() seems too generic of a name. Maybe get_type_names()? Also, what we really want is a string to presnt to the user, so consider returning a string from the method

    return repr(type_names)
    

    If you change th would name this method name to something like format_type_tuple or something like that.

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

    Although this is perfectly acceptable python syntax, creating a list often gets written in short form.

    type_names = [ item.__name__ for item in types_tuple ]
    

    This is called a list comprehension. You can use a similar trick for dictionaries.

  4. 
      
JR
JR
JS
  1. 
      
  2. src/python/pants/option/custom_types.py (Diff revisions 1 - 2)
     
     

    Eric's suggestion is still a bit cleaner and more idiomatic. Adapted to format - gets you:

    return ' '.join(item.__name__ for item in type_tuple)
    
  3. 
      
ZU
  1. 
      
  2. src/python/pants/option/custom_types.py (Diff revision 2)
     
     

    Looks like I forgot to hit publish and John had the same suggestion. There is a better way to format a list as a string with separators, use the string.join() method. Of course, there's more than one way to do it!
    Here's putting it all together with a list comprehension:

    return ', '.join([ item.__name__ for item in type_tuple ])
    

    See a relevant stack overflow question http://stackoverflow.com/questions/497765/python-string-joinlist-on-object-array-rather-than-string-array for a discussion of different options and performance implications.

  3. 
      
JR
JS
  1. I'll leave it to Eric to patch this in.
  2. 
      
AR
  1. Awesome, thanks for fixing this!

  2. 
      
JR
ZU
  1. Committed to master at dfc25b1. Please mark this review as submitted!

  2. 
      
JR
Review request changed

Status: Closed (submitted)

Change Summary:

dfc25b1

Loading...