Handle broken resource mapping files

Review Request #2038 — Created April 7, 2015 and submitted

dturner-tw
pants
65bc91a...
pants-reviews
jsirois, stuhood

Handle broken resource mapping files (by throwing exceptions).

Ran new tests locally.

CI running: https://travis-ci.org/pantsbuild/pants/builds/57559852

JS
  1. 
      
  2. I like Eric's style of exception subclass per unique failure type.  In general its only for tests, but it is nice to know you're getting the exact failure you expect.  As you see fit.
    1. I learned that convention at Google: https://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Exceptions

    2. Great idea. Going to try to do that from now on.

  3. 
      
DT
JS
  1. Another bit to take or leave.
  2. I prefer Twitter style [1] here, ie: nest exceptions in the class that raises them when the class is the only source of the exceptions. This allows for less imports. For example in your tests below you'd kill the exception imports and:

    ...
    resource_mapping = ResourceMapping(rel_dir)
    
    with self.assertRaises(resource_mapping.TruncatedFileException)
    ...
    

    [1] https://github.com/twitter/commons/blob/master/src/python/twitter/common/styleguide.md#best-practices - Item #6

  3. 
      
IT
  1. looks good! John's suggestion to nest exception classes is also my preferred way if that helps in decision making!

  2. 
      
DT
JS
  1. That was a diffbomb
    1. oops, rbt hates me

  2. 
      
DT
JS
  1. Ship It!
  2. 
      
DT
Review request changed

Status: Closed (submitted)

Loading...