Add proper package handling for antlr4 targets

Review Request #298 — Created April 30, 2014 and submitted

jcoveney
pants
jco/antlr_package
pants-reviews
jsirois
Add proper package handling for antlr4 targets
I've tested it locally with grammars we use internally. I might add that before this patch, using the antlr4 compiler was broken and couldn't be used at all.
  • 0
  • 0
  • 1
  • 0
  • 1
Description From Last Updated
JS
  1. Thank you - only blocking issue is marked as such.
    
    Since I don't have nice test infra to point you at for this - the sample addition to the pantsbuild/pants tree to be picked up by ci's `./pants goal test {src,tests}/{java,scala}::` or some augmentation of that ci bit is welcome.  
  2. src/python/pants/tasks/antlr_gen.py (Diff revision 1)
     
     
    No where else - except in maven_layout(...) do we try to be like maven, please kill this clause.
  3. src/python/pants/tasks/antlr_gen.py (Diff revision 1)
     
     
    Do you know how the flag interacts with a package set in the grammar?  We _could_ get this from non-maven conversions going forward.
    1. It'll error out, I believe. the @header {} allows the users to input arbitrary code. If there is a package declared there, then it will mess up here.
      
      That said, the ANTLR guys have said it's basically an antipattern now to put the package declaration there (mostly because the goal is to have grammars be as language agnostic as possible).
    2. The antipatterns will get used and we'll get the bug report - we know this well - so the error bit sounds great.
  4. src/python/pants/tasks/antlr_gen.py (Diff revision 1)
     
     
    We deviate from pep8 in 2 respects only - 100 cols max, 2 space indents. http://legacy.python.org/dev/peps/pep-0008/
    1. Do we have an intellij profile for pants development?
    2. Not that I'm aware of - I think everyone sets this up manually today.
  5. src/python/pants/tasks/antlr_gen.py (Diff revision 1)
     
     
    Or just snap a set([...]) and check singleton-ness
  6. 
      
JC
JS
  1. 
      
    1. It seems like one of y'all have to merge this? It's ready for you.
  2. src/python/pants/tasks/antlr_gen.py (Diff revision 2)
     
     
    Missed this 1st time - adjacency (ws ignored) does a concat for strings in python) - its idiomatic to drop the + here since you're inside parens.
  3. 
      
JC
JS
  1. In: https://github.com/pantsbuild/pants/commit/92a3b138e133c390ec6b0c697431957bc7484ad7
    
    Please mark this RB submitted.
    
    Thanks for the contribution.
  2. 
      
JC
Review request changed

Status: Closed (submitted)

Loading...