PEP8Bear.py: Show PEP8 error description #1898
Conversation
JohnMoutafis
force-pushed
the
1897-show-pep8-error-description
branch
from
bb2e3f3
to
df480d1
Compare
JohnMoutafis
force-pushed
the
1897-show-pep8-error-description
branch
from
df480d1
to
563a41e
Compare
|
Comment on c23ba92. Shortlog of the HEAD commit contains 59 character(s). This is 9 character(s) longer than the limit (59 > 50). GitCommitBear, severity NORMAL, section |
|
Oops! That's ^ what you get when you push all the buttons!! |
JohnMoutafis
force-pushed
the
1897-show-pep8-error-description
branch
from
c23ba92
to
fdd6e0d
Compare
|
Missing tests :( |
| @@ -41,14 +42,69 @@ def run(self, filename, file, | |||
| 'max_line_length': max_line_length, | |||
| 'indent_size': indent_size} | |||
|
|
|||
| errors = self.list_pep8_errors(source=file, options=options) | |||
There was a problem hiding this comment.
If I understand this correctly, you run pycodestyle independently and then match it with result from autopep8?
I think it will be unreliable because the coala-generated diffs below is not guaranteed to match 1-on-1 with list from pycodestyle.
There was a problem hiding this comment.
I can confirm that. The documentation states that autopep8 fixes some things not reported by pycodestyle
There was a problem hiding this comment.
autopep8.fix_code method fixes the errors reported by pycodestyle.
If you check autopep8's code you will see that it runs a pycodestyle check and then it applies the fixes on the reported errors.
There was a problem hiding this comment.
Yes, but as I said it does more than that, for example lib2to3.refactor
|
It seems that you have put in quite an effort into this. Thank you for doing that! However, I don't think this is the right approach. It makes many assumptions and does quite a bit of work to fix an issue that could easily be fixed by the upstream tool. Have we ever tried to go upstream and patch autopep8 to actually report the error being fixed? I'm also curious to hear other peoples oppinion! |
bears/python/PEP8Bear.py
Outdated
| yield Result(self, | ||
| 'The code does not comply to PEP8.', | ||
| 'The code does not comply to PEP8.\nReason: {}' | ||
| .format(errors[i]['info'] if i < len(errors) else ''), |
There was a problem hiding this comment.
'' -> 'Unknown' would make more sense to me FWIW
|
Generally I agree with @NiklasMM - this is awesome stuff but it's nondeterministic. It is already known that our diff splitting isn't always correct so we know there will be cases where this'll fail. We should be really sure if an upstream contribution wouldn't be possible, cause upstream is generally better for the community anyway. @JohnMoutafis any thoughts on that? |
|
(marking WIP in the meantime) |
Add PEP8 error message description for every error
found by autopep8 during PEP8Bear analysis.
Utilizes pycodestyle (as autopep8).
Best case senario:
- errors list and diffs list have the same size.
Worst case scenario:
- errors list is sorter than the diffs list:
The loop will utilize every error and then will print an empty
line bellow the 'does not comply' message.
- errors list is empty:
The loop will only print the 'does not comply' message.
- errors list is larger than diffs list:
The loop will print every diff and the corresponding error.
Closes coala#1897
JohnMoutafis
force-pushed
the
1897-show-pep8-error-description
branch
from
fdd6e0d
to
737bad9
Compare
|
I have added some explanation in the commit body. |
|
interesting: apparently there's a |
|
this verbose flag is interesting, it already gives you the lines affected per issue class - so with that we'd be able to get a much more reliable mapping right? |
|
So if I understand your patch right it assumes that the errors are in the same order, meaning if our patch splitting is wrong all the results get shifted by one and the messages will all be wrong, right? That won't be the case when relying on the lines. |
|
@sils The |
Add PEP8 error message description for every error
found by autopep8 during PEP8Bear analysis.
Closes #1897