Convert ragel-gen.py to use new options and expunge config from BinaryUtil.

Review Request #1970 — Created March 21, 2015 and submitted

zundel
pants
zundel/ragel-gen-options
1308
8435ddf...
pants-reviews
benjyw, jinfeng, jsirois

- Register global options --pants-support-baseurls, --max-subprocess-args,
and --pants-support-fetch-timeout-secs.
- Remove 'config' parameter and other references from BinaryUtil.
- Remove pants.ini values that have good compiled in defaults.
- Updated help for the common --support and --version flags by making BinaryUtil a mixin

See Bug link for latest CI status.

  • 0
  • 0
  • 1
  • 0
  • 1
Description From Last Updated
ZU
ZU
  1. 
      
  2. I essentially added this method only as a place to hang a hook on the comment that its options come from another task.

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

    I spent some time trying to make BinaryUtil a mixin, but ultimately gave up on that. I ran into issues because not all callers of BinaryUtil are task subclasses.

  4. src/python/pants/binary_util.py (Diff revision 1)
     
     

    I removed all of this setting of private attributes when they are just as easy to fetch from the options object.

  5. src/python/pants/binary_util.py (Diff revision 1)
     
     

    I didn't see any references to binary_base_strategy anywhere so I removed it thinking it was just out of date.

  6. src/python/pants/binary_util.py (Diff revision 1)
     
     

    I did a lot of renaming from 'base_path' to 'supportdir' to line up with the option name.

  7. I made this recursive primarily to make it easy to get to from BinaryUtil methods. If there were a way to fetch the global options scope without a task reference, this wouldn't have been necessary.

  8. This is recursively registered for the same reason as --pants-support-baseurls

  9. What do you think of this? BinaryUtil is supposed to be a task helper, so I created a dummy task to help test it - mainly to get the options values from it.

  10. I moved this function back inside of the main unit test. It isn't shared by any other tests.

  11. These URLs were out of date, so I updated them. I figured wouldn't make sense to someone who didn't know the history of where we've hosted tool binaries.

  12. 
      
ST
  1. 
      
  2. This is inconsistently constanted vs bin/thrift... IMO, can just inline if you're not using in multiple places: there is plenty of context here to explain what it is.

  3. src/python/pants/backend/codegen/tasks/ragel_gen.py (Diff revision 2)
     
     
     
     
     
     
     

    Sidenote: These repeated options are ripe for Benjy's subsystems idea.

  4. src/python/pants/binary_util.py (Diff revision 2)
     
     
     

    Looks like all of these methods could be static at this point? Or at the very least, @classmethod?

    1. If everyone concurs we don't need init anymore, I will make this change.

  5. src/python/pants/binary_util.py (Diff revision 2)
     
     

    Should these methods consistently take an options instance?

    1. Great minds think alike. I did that at first, but then ran into ugliness in the unit testing. The unit test shoves a bunch of different values for supportdir, version, and name through this function so I think keeping them as parameters is the best choice.

  6. src/python/pants/binary_util.py (Diff revision 2)
     
     
     

    rather than reusing the variable name, would be good to rename these to indicate what differentiates them

    1. I just inlined it all into one line.

  7. 
      
ZU
JS
  1. 
      
  2. How about `BinaryUtil.from_options(options)` that calls a constructor that takes explicit args.  A test then just calls the constructor with explicit args instead of going around the bend to plumb options through a Task - which is a red-flag import for a test of a non-task.
    1. The problem isn't in BinaryUtil, it was in getting all the options setup properly in the test. I think I can simplify the test and remove the task now that I've gotten all the bugs worked out.

    2. Even so, consider the options factory.  At the end of the day I think its almost always better to move a configuration subsystem out to the periphery and have objects that are fundamentally useable on their own without auxiallary parameter setup systems.
    3. I'll ponder it a bit. Stu and I were thinking that maybe we'd get rid of the instance methods on BinaryUtil all together. This suggestion moves in the opposite direction.

  3. 
      
ZU
JS
  1. 
      
  2. src/python/pants/binary_util.py (Diff revisions 3 - 4)
     
     
    Consider moving the constructor down below the class methods.
  3. src/python/pants/binary_util.py (Diff revision 4)
     
     
    The docs were nice to have, consider bringing them back.
  4. 
      
ZU
ZU
Review request changed

Status: Closed (submitted)

Change Summary:

Thanks for the reviews Stu and John. Commit 5b230fd

BE
  1. 
      
  2. I just noticed that these are curly braces, not parens, so the key here is a set literal, not a pair.

    1. Oops, see https://rbcommons.com/s/twitter/r/1995/

  3. 
      
Loading...