-
Notifications
You must be signed in to change notification settings - Fork 56
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix linter output parsing on Windows #378
Conversation
@deadkex I started by creating a related test case. Let's see how it behaves on the Windows test runner. Would you be willing to do a code review once I get a fix done? Could I add you as a contributor to this repository so I can assign the review to you? |
Sure |
Update: Fixed in #379. In some of the runners we get this error:
Based on the traceback, what is going on is similar to this snippet from a Python shell: >>> path_from_cwd = Path('/home/runner/work/darker/darker/.pylintrc')
>>> root = Path('/home/runner/work/darker/darker/src/darker/tests')
>>> path_from_cwd.relative_to(root)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "lib/python3.10/pathlib.py", line 816, in relative_to
raise ValueError("{!r} is not in the subpath of {!r}"
ValueError: '/home/runner/work/darker/darker/.pylintrc' is not in the subpath of '/home/runner/work/darker/darker/src/darker/tests' OR one path is relative and the other is absolute. So The error is from a |
Update: Fixed in #379. In the failing builds we have |
Update: Fixed in #379. I reproduced the
|
Update: Fixed in #379. And indeed:
So the But the second line points to the Pylint configuration file! This is where Darker gets very confused, because that file is outside the common root of files given on the command line. We probably need to skip all linter output lines referencing such files. |
6c7b4a0
to
1fa2f6e
Compare
At least in the added test case, I'm betting the problem is the colon after the drive letter in
|
@deadkex I believe the issue is now fixed. Could you try this branch with your original setup on Windows? |
Looks good on my personal pc/project so far.
|
Great, let's wait until then before merging. |
778dc6d
to
c6cf040
Compare
Okay so i noticed that the src and lint parameter might not get parsed/put together correctly when not specifying a linter in the console:
|
When using
|
Edit: Edit2: Original comment:
|
@deadkex this is clearly wrong. The |
@deadkex also here something's odd: lint = [
" pylint\n --rcfile=tools/pylint.conf\n ",
" flake8\n --ignore=E501,F841\n --max-line-length=80\n ",
"test.py",
] Since your
(assuming that your source tree contains the two Python source code files
Edit: Ah, the config dump was probably output from |
Yes. Just leave out |
File "c:\python38\lib\site-packages\darker\multiline_strings.py", line 20, in <listcomp>
return [
File "c:\python38\lib\tokenize.py", line 521, in _tokenize
raise TokenError("EOF in multi-line statement", (lnum, 0))
DEBUG:darker.linting:[C:\Users\deadkex\folder\project]$ pylint --rcfile=tools/pylint.conf C:\Users\deadkex\folder\project\test.py
DEBUG:darker.linting:Unparsable linter output: ************* Module tools\pylint.conf
DEBUG:darker.git:[C:\Users\deadkex\folder\project]$ git show HEAD:./tools/pylint.conf
DEBUG:darker.git:[C:\Users\deadkex\folder\project]$ git log -1 --format=%ct HEAD -- tools/pylint.conf And indeed, Darker is trying to parse Could you run You know, if I'm not mistaken, this is exactly the kind of edge case which yesterday's commit "Make linter output parsing stricter" should prevent. Did you pull the branch before doing these tests? If not, could you do that and see if it helps? |
c6cf040
to
a2c8941
Compare
Yep, my bad i was overthinking it. |
Yes
And let me guess darker doesn't like that pylint outputs config errors as normal linting errors... |
I encountered another thing.
Edit: I guess darker just doesn't return anything if there are no problems? |
Alright, so we really need to filter out non-Python file paths as well as paths outside the common root of file paths requested to be checked? This would follow the principle of "only showing messages which fall on modified lines of Python files". Or,, does it make more sense not not filter them out? In this case the principle would be to "filter out messages which fall on unmodified lines of Python files". Anyway, it shouldn't crash, so I'll add this to the checklist in the description of this PR. |
Hmm if those errors don't get displayed the user wouldn't know that there's something wrong with their config. I also only found out because i ran pylint with its config directly via pre-commit and it was shown there. I guess running pylint directly would also work but i don't know. |
@deadkex, the Could you see how this affects your results? |
13f52d6
to
d7b5385
Compare
So @deadkex would you consider all the issues you've raised as solved by this PR? |
- no leading/trailing spaces in file path - nothing but digits in line and column number (not even +/-) - strip trailing whitespace from description
fafa6a5
to
611fffd
Compare
Fixes #374.
.py
files