-
Notifications
You must be signed in to change notification settings - Fork 992
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
Python Flake8 linter workflow to replace stickler-ci #1786
Conversation
Stickler CI appears to only lint the functions changed inside a file, but this workflow lints the entire file. I am working on changing it to only add annotations for changed functions. |
Personally, I don't like excluding errors or warnings for all files, especially For reference, these are currently excluded in this PR:
flake8 does not provide a way to disable errors for code blocks, but per file with a configuration file (or per line as we know). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR looks great! I didn't know (or maybe forgot) that jobs could create annotations like this. Very cool. Seems to work as intended.
One thing to note is that this job flags all errors in the changed files, whereas I think (although my memory is a bit hazy) our former setup with stickler-ci wouldn't flag unchanged lines. The old behavior had pros and cons: it wouldn't pester the contributor with errors unrelated to their PR, but it also wouldn't necessarily catch things like unused imports. In our case, flagging all errors means making a tiny change to some files will result in dozens of errors. See for example: https://github.com/pvlib/pvlib-python/actions/runs/5476853711/jobs/9975004553?pr=1786
Is there some way to not report those unrelated errors? Unless we go through and fix all of those (which I'm not sure is a good idea), I think they will make this CI job a lot less useful for many PRs.
Something else is that there is a maximum of 10 annotations. Not really a big deal. You can always look at the job text output to see the full list of lint errors.
For reference, these are currently excluded in this PR
I think @reepoi just copied those exclusions over from our current lint configuration in .stickler.yml
. We can certainly talk about changing those rules, but let's focus on just getting the linter running again in this PR.
do we want to use an older version of flake8 that supports the deprecated |
Using an older version of flake8 that supports the As a bonus, flake8's |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Does anyone else have any comments before we merge this?
@reepoi can you make a short 0.10.2 whatsnew entry and list yourself as a contributor? And remove the demonstration lint error of course :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My only and entirely optional comment is whether we want to continue ignoring E226, Missing whitespace around arithmetic operator. I find the whitespace makes equations easier to read.
I think whitespace is often but not always an improvement. For example, I prefer no whitespace around the pvlib-python/pvlib/temperature.py Line 752 in 6606af7
I lean towards continuing to ignore that linter rule, but I don't feel strongly about it. |
@kandersolar @cwhanse Idk if Kevin's point is only for exponentiation operator, but E226 does not raise warnings for operator Just wanted to point that out, with reviewers doing their job there shouldn't be any unreadable code pushed. |
Thanks @echedey-ls, that's good to know! The Anyway I'm going to go ahead and merge this so that we have an operational linter again. Thanks @reepoi for setting this up and everyone else for reviewing! |
[ ] Tests added[ ] Updates entries indocs/sphinx/source/reference
for API changes.docs/sphinx/source/whatsnew
for all changes. Includes link to the GitHub Issue with:issue:`num`
or this Pull Request with:pull:`num`
. Includes contributor name and/or GitHub username (link with:ghuser:`user`
).remote-data
) and Milestone are assigned to the Pull Request and linked Issue.Adds GitHub workflow to replace stickler-ci for Python code linting with flake8. The new workflow annotates the PR changes using the errors outputted by flake8. Here is an example showing what the annotations look like: https://github.com/reepoi/pvlib-python/pull/1/files
The workflow implementation is based on the GitHub action here: https://github.com/pr-annotators/flake8-pr-annotator.