Skip to content

respect noqa#182

Merged
Zac-HD merged 1 commit intopython-trio:mainfrom
jakkdl:noqa
Apr 30, 2023
Merged

respect noqa#182
Zac-HD merged 1 commit intopython-trio:mainfrom
jakkdl:noqa

Conversation

@jakkdl
Copy link
Member

@jakkdl jakkdl commented Apr 28, 2023 •
edited
Loading

fixes #181
Implemented:

  • respects noqa
  • new pyright error about how to in-line silence stuff.
    • it would be nice to catch errors-due-to-updated-linters with a bot, instead of me randomly encountering them when writing new PRs and being surprised.
  • various small test improvements
    • changes parametrized params to test_eval so summaries are more informative than e.g. test_eval[False, True, TRIO100, path7]
    • sort errors on columns late, since columns have a habit of getting messed up and thrown around (and might be ignored anyway)
    • ignore columns when checking with autofix
      • the scenario here is two errors on the same line, the first which modifies the line, causing the second to have a different column

remaining TODO:

  • don't autofix noqa'd errors
    • ... ah shit, I have to kind of completely change when errors get suppressed to fix this - since visitors have to know if an error is noqad when they raise/autofix it.
  • support noqa-trio (or similar / depends on discussion in Support # noqa comments when calling directly #181)
    • handle NQA102 for noqa-trio
      • separate error, or command-line flag, if code not enabled
    • check that ignored code is valid/exists
      • maybe give separate error if it's not enabled, see above
  • autofixing two errors in the same line seems broken
    • TRIO100 autofix looks like it's eating comments
    • TRIO911 autofix does not get executed, although that's possibly due to it being a SimpleStatementSuite
    • might fix these in a separate PR which i rebase this onto, or just do NOAUTOFIX
  • noqas with invalid code is parsed as bare noqa (but that's same as flake8, so /shrug )
  • report old regex101.com link upstream in PyCQA/flake8
  • don't suppress errors when running as a plugin
  • --disable-noqa

@jakkdl jakkdl mentioned this pull request Apr 28, 2023
Open
@Zac-HD
Copy link
Member

Zac-HD commented Apr 29, 2023 •
edited
Loading

it would be nice to catch errors-due-to-updated-linters with a bot, instead of me randomly encountering them when writing new PRs and being surprised.

Yeah, I think it's time that we started pinning our dependencies. Steal the pip-tools setup from shed in a new PR? The idea is to pin all the transitive deps, and then have a tox env that updates them - for now we can just run that manually every so often.

@Zac-HD
Copy link
Member

Zac-HD commented Apr 29, 2023

don't autofix noqa'd errors

For simplicity I think we can skip this too for now; if it's autofixable I think we're OK with doing that and if I turn out to be wrong, well, that's a future problem 😝

Zac-HD
Zac-HD reviewed Apr 29, 2023
View reviewed changes
Copy link
Member

@Zac-HD Zac-HD left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good!

tests/test_flake8_trio.py Outdated
Comment on lines 194 to 197
# pytest prints parametrized values, but not the key, so to make the summary info
# readable we make the parameters strings and here turn them into bools
autofix = autofix_str == "autofix"
anyio = library == "anyio"
Copy link
Member

@Zac-HD Zac-HD Apr 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's also the ids= argument which can customize this, passing either a list of strings or an arg->str function.

Copy link
Member Author

@jakkdl jakkdl Apr 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ooh, much better!

@jakkdl
Copy link
Member Author

jakkdl commented Apr 30, 2023

don't autofix noqa'd errors

For simplicity I think we can skip this too for now; if it's autofixable I think we're OK with doing that and if I turn out to be wrong, well, that's a future problem stuck_out_tongue_closed_eyes

well, if you have any TRIO91x errors noqa'd they'll be autofixed - although I suppose extra lowlevel checkpoints won't do anything. noqa'd TRIO100 being autofixed is quite a bit worse, but maybe not too common. But I can postpone it to a separate PR regardless.

@jakkdl
Copy link
Member Author

jakkdl commented Apr 30, 2023

Oh actually, current solution is very incompatible with flake8-noqa as it suppresses errors even when run non-standalone...

@jakkdl jakkdl marked this pull request as ready for review April 30, 2023 18:37
@jakkdl
Copy link
Member Author

jakkdl commented Apr 30, 2023

But feel free to merge and test against live code, and if it works up to you how badly you want to break flake8-noqa, or wait until followup PRs.

@jakkdl
Copy link
Member Author

jakkdl commented Apr 30, 2023 •
edited
Loading

nowait, this also needs to ignore ast visitor errors. but can fix that in another PR

This was referenced Apr 30, 2023
Merged
Merged
Closed
@Zac-HD
Copy link
Member

Zac-HD commented Apr 30, 2023

Looks like we've got some useful fixes here for the other PRs, so merging this + #184 now. I'll test locally ~monday (I hope 🤞), and cut a release either then or after #185 if that turns out to be really necessary. Thanks again!

@Zac-HD Zac-HD merged commit 5aaaaad into python-trio:main Apr 30, 2023
@jakkdl jakkdl deleted the noqa branch May 1, 2023 09:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Zac-HD Zac-HD Zac-HD approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Support # noqa comments when calling directly

2 participants

@jakkdl @Zac-HD