pythonstyle: Fix suppression support; improve SyntaxError reporting; Only report each nit once

Review Request #3647 — Created April 4, 2016 and submitted

nhoward_tw
pants
3128, 3139
3806
pants-reviews
benjyw, gmalmquist, jsirois, molsen

This patch cleans up a number of issues I encountered while trying to fix suppression support in the Python style checks.

  • The suppression option wasn't working as expected because the file names passed to it were changed from relative to absolute. This moves parsing into get_nits so that it has access to the relative file path.
  • SyntaxErrors show only the default error message without any source context. This patch adds source context display to syntax errors.
  • Multiline nits are counted and outputed once for each line they contain. This changes it so that they are only counted and logged once.

The one change I want to call out specifically is that I changed Nit so that it doesn't know about the PythonFile anymore. I did this so that I could make a nit for SyntaxErrors without introducing a variant of PythonFile for syntactically incorrect Python files.

If there are external plugins that rely on Nit's current structure, this may break them.

Wrote a test covering using suppression, and fixed it. Then added more tests around syntax error handling and fixed those.

After doing some testing, I noticed the multi-line display issue and fixed that as well following the same process.

CI away on the PR.

  • 0
  • 0
  • 2
  • 1
  • 3
Description From Last Updated
  1. 
      
  2. Python has some strange behavior with os.path.join:

    In [1]: import os

    In [2]: os.path.join('foo', '/bar/baz')
    Out[2]: '/bar/baz'

    In [3]: os.path.join('foo', 'bar/baz')
    Out[3]: 'foo/bar/baz'

    We should probably check for this case and raise an error if both root is specified and an absolute path is used. I don't think we actually want users doing that.

  3. I would rather this and PythonFile inherit from an abc so we can force these methods to exist. It also makes it a little clearer to me the intent of this class.

  4. 
      
  1. 
      
  2. Bit confusing to rename this variable without renaming the underlying option.

    1. I found it confusing next to fail_threshold in the loop below as it serves a similar purpose but that's not reflected in the name.

      It's interesting that we have severity, strict and fail. I feel like strict and severity could interact poorly and sort of overlap with fail. For example, you could set severity to ERROR, but turn on strict, which would fail on warnings but not print them.

      Maybe it'd make more sense to have a log-level and a fail-level. Then we could validate that logged types are a superset of fail types, and if you wanted to turn off failure on style, but still warn, you could leave off a fail-level.

  3. Can you add a comment explaining why this won't collide with error numbers that the checks themselves might implement?

  4. 
      
  1. looks good to me

  2. Thanks for catching this bug, looks like this must have accidentally gotten indended somehow.

  3. 
      
  1. Ship It!
  2. 
      
  1. Ship It!
  2. 
      
Review request changed

Status: Closed (submitted)

Change Summary:

https://github.com/pantsbuild/pants/commit/8479b6a979a0433c9b360c340fca454f03b0ff77

  1. FYI, I'm getting an exception I think is related to this change after upgrading to 0.0.81. See https://github.com/pantsbuild/pants/issues/3188

  2. 
      
Loading...