Python Flake8 linter workflow to replace stickler-ci

[reepoi]

• Closes stickler-ci is shutting down #1722, closes Add lint checks to CI (without Stickler CI) #776
• I am familiar with the contributing guidelines
• [ ] Tests added
• [ ] Updates entries in docs/sphinx/source/reference for API changes.
• Adds description and name entries in the appropriate "what's new" file in 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`).
• New code is fully documented. Includes numpydoc compliant docstrings, examples, and comments where necessary.
• Pull request is nearly complete and ready for detailed review.
• Maintainer: Appropriate GitHub Labels (including 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.

[reepoi]

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.

[echedey-ls]

Personally, I don't like excluding errors or warnings for all files, especially W504.

For reference, these are currently excluded in this PR:

• E201, Whitespace after '('
• E226, Missing whitespace around arithmetic operator
• E241, Multiple spaces after ','
• W503, Line break occurred before a binary operator
• W504, Line break occurred after a binary operator

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).
I'm not suggesting changing current behaviour, I only want to point that out.

[kandersolar]

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.

[wholmgren]

do we want to use an older version of flake8 that supports the deprecated --diff option?

[reepoi]

Using an older version of flake8 that supports the --diff option is a good idea, and I changed the workflow to do that. If we didn't, we'd need to find the changed Python files and filter their linting errors by line number. The easiest I found to get the line numbers of changes in a file is by parsing the output of git diff, which is what the older versions of flake8 do.

As a bonus, flake8's --diff option appears to ignore changes in files that are not Python files automatically. The workflow's code to do that is removed now.

[kandersolar]

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 :)

[cwhanse]

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.

[kandersolar]

I think whitespace is often but not always an improvement. For example, I prefer no whitespace around the ** operators here:

pvlib-python/pvlib/temperature.py

Line 752 in 6606af7

tground = (tinoct**4 - backrat * (tinoct**4 - 293.15**4))**0.25

I lean towards continuing to ignore that linter rule, but I don't feel strongly about it.

[echedey-ls]

@kandersolar @cwhanse Idk if Kevin's point is only for exponentiation operator, but E226 does not raise warnings for operator **. I've also tested it to be sure with latest flake8.

Just wanted to point that out, with reviewers doing their job there shouldn't be any unreadable code pushed.

[kandersolar]

Thanks @echedey-ls, that's good to know! The ** operator was the first that came to mind, but I expect there are other cases where we'd like to not be hassled about operator whitespace too.

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!