Remove direct config access in scalastyle.py.

Review Request #1897 — Created March 11, 2015 and submitted

benjyw
pants
9f9ff17...
pants-reviews
ity, jinfeng, jsirois
Moved validation of task inputs to helper functions called in execute(),
as it's a bad idea to throw in a task's __init__ method.

Note that test_scalastyle.py does a lot of poking around in Scalastyle's
internal state, which is not recommended in general and is defeated by
moving the validation out of __init__ as mentioned above.

So this change also does some refactoring of scalastyle.py to make it
more testable.

I also removed a couple of tests that were literally just testing that
internal variables were set from config, for similar reasons.

CI passes: https://travis-ci.org/pantsbuild/pants/builds/53888857

  • 0
  • 0
  • 1
  • 0
  • 1
Description From Last Updated
BE
BE
JS
  1. 
      
  2. An outright skip check seems like it belongs 1st.
  3. Indent the k/v pairs by 2as you do below on line 67.
  4. You might consider TaskError subclasses per-each to help prove you're testing what you think you're testing.  Eric has prompted me on this in the past and I've warmed to it.
    1. Yeah, that's nice. Done.

  5. 
      
NH
  1. 
      
  2. I think using any() here might be easier to read.

    return not any(exclude.match(source_filename) for exclude in self.excludes)
    
  3. Update the docstring to refer to the new config section.

    1. Fixed by enhancing the option help and removing this part of the comment entirely.

    2. Even better.

  4. should these be advanced options? They seem like the kind of thing you'd only set once within a repo.

  5. 
      
BE
BE
Review request changed

Status: Closed (submitted)

Change Summary:

Submitted as a5c194eb37b31f00d13ebf7991767bf19d3fd332.

JS
  1. Ship It!
  2. 
      
NH
  1. Ship It!
  2. 
      
Loading...