The public API for the new options system.

Review Request #831 — Created Aug. 5, 2014 and submitted

benjyw
pants
pants-reviews
jsirois
The public API for the new options system. Relies on a bunch of code reviewed by others and pushed to master, so feel free to browse that for context. All relevant code is in src/python/pants/option and tests/python/pants_test/option/.

This completes the new options code. Remaining work involves actually hooking this code up to something...
Unittests pass. CI baking. This code isn't hooked up to anything yet anyway.
JS
  1. Sorry for the delay here.  Exciting!
    1. Thanks for the review!
  2. src/python/pants/option/options.py (Diff revision 1)
     
     
    Unused in this review - is this an artifact of review staging?.  If it is used somewhere I always prefer a docstring for the class to a pass.  Are these raised for ill defined options or badly formed option args on the command line or both?, ie I like:
      class OptionError(Exception):
        """Indicates ..."""
    1. Looks like it's truly unused. Removed. Agreed re docstring, although I also like to add 'pass' as it indicates to the reader that I didn't forget to implement but truly intended the class/method to be empty.
  3. src/python/pants/option/options.py (Diff revision 1)
     
     
    +1 indent for this line and the next
    1. Ooops, good catch. Fixed. 
  4. If you import unittest2 then you get the native with self.assertRaises(...):
    1. Good to know. Changed here, but we have a lot of places where we do the pytest thing.
      
      Also, unclear to me whether we should 'import unittest2' or 'import unittest2 as unittest'. We seem to do both. 
      
      Anyway, hopefully all should be clarified in your testing infra cleanup+documentation?
  5. 
      
BE
Review request changed

Status: Closed (submitted)

Loading...