A hierarchy of argparse parsers.

Review Request #819 — Created Aug. 1, 2014 and submitted

benjyw
pants
pants-reviews
patricklaw
Each parser corresponds to a command-line scope (e.g., 'compile.java').

See the code for copious comments.

Note: A future change will contain comprehensive tests for this code's
functionality.
Tests are in a separate change, via a wrapper class that exposes this code's public API.
  • 0
  • 0
  • 7
  • 1
  • 8
Description From Last Updated
PA
  1. Are there any tests for this?
  2. src/python/pants/option/parser.py (Diff revision 1)
     
     
    This seems mildly scary.  It's messing with a parent's private attribute, which is a red flag, and we've seen issues with this pattern of registration before.
    
    It might be better to have an explicit `register_child_parser` method on Parser so at least we can log the order of events and expose an explicit public interface.
    1. Yeah, I don't love this either, but the trouble is, this shouldn't be in a public interface. I don't want other classes calling "register_child_parser", and I don't want readers of this file to think they can call it.  This is an internal implementation detail of this class. But the parent is of the same class as the child, so this is fine from an encapsulation perspective. C++ and Java would allow this on a private attribute. Scala wouldn't if you used path-dependent types, but that's getting pretty far removed from the python type system...
      
      What I can do as a compromise is turn these direct attribute settings into method calls, but with private names, e.g., _register_child_parser or whatever. I've made that change, see if you like it any better.
    2. Yeah, the C++/Java alternative is what was going through my head when I was reading this, but it still bothered me.  I like your compromise, I think it fixes the general scariness of modifying private instance attributes that you don't directly own.
  3. src/python/pants/option/parser.py (Diff revision 1)
     
     
    .format
  4. src/python/pants/option/parser.py (Diff revision 1)
     
     
    Again, might be worth exposing a `freeze` method.
    1. Done, but named _freeze() for the same reasons as above.
  5. src/python/pants/option/parser.py (Diff revision 1)
     
     
    Can you elaborate on this?  It's not clear to me what's happening here.
  6. src/python/pants/option/parser.py (Diff revision 1)
     
     
    Are there any corner cases where flag can be shorter than 3 characters?  If not, can you drop a comment explaining why?
    1. Note the flag.startswith('--') condition on the previous line. flag[2:] is guaranteed not to throw. And we know that a flag is at least one letter, so... I think a comment would be superfluous.
    2. Doh, should have noticed that.  I agree
  7. src/python/pants/option/parser.py (Diff revision 1)
     
     
    .format
  8. src/python/pants/option/parser.py (Diff revision 1)
     
     
  9. src/python/pants/option/parser.py (Diff revision 1)
     
     
  10. 
      
ST
  1. 
      
  2. src/python/pants/option/parser.py (Diff revision 1)
     
     
    s/exists/exits/
  3. src/python/pants/option/parser.py (Diff revision 1)
     
     
     
     
     
     
    Would be more helpful if these were complete/working examples... Since there isn't a compile.scala `goal`, you'd need to do something like:
    `./pants goal compile compile.scala --foo $targets`
    ... right?
    
    Or does requesting the compile.scala scope automatically trigger the parent scopes/goals?
    1. The compile.scala scope triggers the compile goal (see design doc).
  4. worth having a constant?
    
    ROOT_SCOPE = ''
    1. I started out with that, but it ended up being ugly, as it's used 1-2 times per file in several different files, and there isn't an obvious home for it. However it is slightly more readable, so I've made this change.
  5. 
      
BE
PA
  1. Ship It!
  2. 
      
BE
Review request changed

Status: Closed (submitted)

Loading...