Skip to content
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

Run linters twice: 1) baseline, 2) current; then show new messages #393

Merged
merged 12 commits into from
Jan 14, 2023

Conversation

akaihola
Copy link
Owner

@akaihola akaihola commented Sep 12, 2022

Hide linter messages which were already present in the baseline. Track unmodified blocks of code which are moved up or down due to other changes, and match linter messages even though the line number might have changed since the baseline.

Fixes #383 and #380.

@akaihola akaihola added bug Something isn't working enhancement New feature or request labels Sep 12, 2022
@akaihola akaihola added this to the 1.6.0 milestone Sep 12, 2022
@akaihola akaihola self-assigned this Sep 12, 2022
@akaihola akaihola added this to In progress in Akaihola's Open source work via automation Sep 12, 2022
@akaihola
Copy link
Owner Author

@PatrickJordanCongenica, you may be interested to try out this branch! It fixes #383 (originally raised by you in #328).

Would you be interested to do a code review as well? Would you accept an invite as a collaborator on the repository so you could be assigned as a reviewer?

I'm going to clean up some commits still though before this is ready for review.

@akaihola akaihola changed the title Run linters twice: 1) baseline, 2) current, then show new messages Run linters twice: 1) baseline, 2) current; then show new messages Sep 17, 2022
@akaihola
Copy link
Owner Author

Hm, let's rethink a bit. With this patch, Darker is only going to show those linter messages which were newly introduced as a result of changes in the code.

But we'd probably want it to also show old linter messages if they happen to fall on modified lines!

@akaihola akaihola force-pushed the baseline-linting branch 4 times, most recently from dc87d79 to 20b05bb Compare November 15, 2022 21:06
@akaihola akaihola modified the milestones: 1.6.0, 1.7.0 Dec 19, 2022
@akaihola akaihola mentioned this pull request Dec 22, 2022
3 tasks
@akaihola akaihola force-pushed the baseline-linting branch 2 times, most recently from b3c66b8 to bf381cc Compare December 25, 2022 17:57
@akaihola
Copy link
Owner Author

we'd probably want it to also show old linter messages if they happen to fall on modified lines

I believe the patch now does this as well.

@akaihola akaihola force-pushed the baseline-linting branch 4 times, most recently from 071adf7 to e8e4c5c Compare December 28, 2022 20:15
@akaihola akaihola force-pushed the baseline-linting branch 5 times, most recently from b6c484d to 62b48fd Compare January 3, 2023 14:01
@akaihola akaihola mentioned this pull request Jan 5, 2023
14 tasks
@akaihola
Copy link
Owner Author

akaihola commented Jan 7, 2023

Update: The issue described below was solved in #453.


The well-known Python 3.7.2 bug #35797 doesn't explain the test failure on windows-latest, 3.7 (see build #1709) – we're running 3.7.9:

Run actions/setup-python@v4
  with:
    python-version: 3.7
    check-latest: false
    token: ***
    update-environment: true
Installed versions
  Successfully set up CPython (3.7.9)
> os.unlink(fullname)
E PermissionError: [WinError 5] Access is denied:
  'C:\\Users\\RUNNER~1\\AppData\\Local\\Temp\\tmp12s99vo_\\test_run_linters_messages_befo1\\.git\\objects
   \\7a\\ea9bf550cfe9c14baf1e82fd65ebf936d81e22'

In a comment to psf/requests-html#249, @ADVEngr had this happen due to failing to provide a proper path for a subprocess:

The displayed error is very misleading, but in my case there was an executable required for a subprocess, and I had only specified the directory containing the executable. Completing the path the the executable solved the issue.

Here in another comment to psf/requests-html#249 is a report from @insanepopeye resembling our case more:

>>> import os
>>> os.chdir(r"C:\Users\Popeye\Desktop\OD\POC\APP\uploads")
>>> test = r"C:\Users\Popeye\Desktop\OD\POC\APP\uploads"
>>> os.remove(test)
Traceback (most recent call last):
File "", line 1, in
os.remove(test)
PermissionError: [WinError 5] Access is denied: 'C:\Users\Popeye\Desktop\OD\POC\APP\uploads'

Could this be because they're trying to remove the current working directory?

In another comment to psf/requests-html#249 @leenjiru pointed to the Stack Overflow question Deleting read-only directory in Python. And like our problem, that question is about rmtree.

This might be a case of Python bug #26660 "tempfile.TemporaryDirectory() cleanup exception if nonwriteable or non-searchable files or directories created".

Maybe it would help to recursively set all created files in the temporary directory read-write at teardown of the git_repo fixture?

@akaihola akaihola force-pushed the baseline-linting branch 9 times, most recently from 53aa565 to 2d14cf9 Compare January 10, 2023 21:16
@akaihola akaihola merged commit 486df53 into master Jan 14, 2023
Akaihola's Open source work automation moved this from In progress to Done Jan 14, 2023
@akaihola akaihola deleted the baseline-linting branch January 14, 2023 09:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
Development

Successfully merging this pull request may close these issues.

Track distant side effects of changes by doing an extra baseline run for linters
1 participant