Enable flake8 checks #1127
Enable flake8 checks #1127
Conversation
github-actions
bot
added
plasmapy.diagnostics
|
Hey, I'll look through this within a few hours :) |
There was a problem hiding this comment.
Okay, first up, once again - thanks for tackling this! :)
A few notes:
- This got me to realize we have a collective habit of using f-strings everywhere... The code is, of course simpler without them, but I wouldn't mind disabling the check and letting people use them "as defaults" in the future (keeping the removals in this PR as is, of course). I'm not sure which way is better, though, so - this is not a strong opinion.
- A few cases where
variable == Falsewas replaced withnot variable, which would make sense in general but doesn't work with numpy arrays in particular. I've made suggestions wherever that happened to replace those. (EDIT: I think I missed a case with the suggestions, but if you apply the one suggestion I made, failing tests should tell us if there's actually any missing case). - There are a bunch of errors still being raised in https://github.com/PlasmaPy/PlasmaPy/pull/1127/checks?check_run_id=2475959193#step:6:2581. I notice:
- The RST210 errors are being found by the
flake8-rst-docstringsplugin (installed in the tests https://github.com/StanczakDominik/PlasmaPy/blob/83e6337031d88380bda740e1ec2f9e71951d9fc6/.github/workflows/linters.yml#L16 ), which I didn't ask you to install through email, as I didn't think you'd be doing them all in one go! So, you've already gone the extra mile here; but we can revert the removals of those exclusions fromsetup.cfgif you'd prefer to keep this PR shorter. We can always tackle those in the future. - A couple of missed f-strings (F541)?
- and a few W605, which I honestly don't understand why it's being raised and I'm absolutely down to keep excluding them unless we gain that understanding somehow! :)
- The RST210 errors are being found by the
All in all, great first PR, and I'm happy to merge it with minor revisions! ;)
github-actions
bot
removed
the
plasmapy.formulary
|
I'm in the phone ATM and it looks like the github app is having some issues right now. I suppose this isn't merging because of merge conflicts? If so I'll clean those up later today. |
Codecov Report
@@ Coverage Diff @@
## master #1127 +/- ##
=======================================
Coverage 96.91% 96.91%
=======================================
Files 70 70
Lines 6930 6930
=======================================
Hits 6716 6716
Misses 214 214
Continue to review full report at Codecov.
|
|
Oooooh. So because of https://github.blog/2021-04-22-github-actions-update-helping-maintainers-combat-bad-actors/ we now have to have approve every single workflow run on a first contributor's PR, rather than approving it on a per-PR basis. That's... just silly, honestly. |
|
I also untagged Erik and Nick because that's just CODEOWNERS being overzealous, here. |
|
Haha, whoops. My previous failing auto-merge triggered despite flake8 failing. My bad! I was already going to do some minor fixes to the CI setup, so I'll fix the two remaining RST errors while I'm at it today. Thanks, @aksalcido! :) |
this follows up on PlasmaPy#1127, where I had an automerge mishap and merged the PR before flake8 started passing (it's not set to "required" right now).
* Add "linters" env to tox This is supposed to incorporate our configuration from .github/workflows/linters.yml, so that it can be easily run locally. * Add linters env to testing * Remove separate linters workflow * Restore RST213, RST306 exclusions in setup.cfg this follows up on #1127, where I had an automerge mishap and merged the PR before flake8 started passing (it's not set to "required" right now). * Change tox path separators to OS-agnostic {/}
Removed majority of the flake8 checks listed in #1116 under # TODO. Also edited the corresponding files that were throwing the flake8 checks once they were no longer being ignored. The only error code that was not handled completely was F541, only some of the F541 errors were handled, but not all. The remaining errors for F541 is for strings that begin with fr"string". Was a bit confused on if this was an intentional design so I wanted clarification before I handled them. I wanted feedback on if this seems good since this is my first contribution to Open Source and feel a bit anxious on if I was working on it incorrectly on my end.
docstrings.