A simple lint_python GitHub Action

[cclauss]

As discussed in #3420 and with @pekkaklarck on Slack, let's land a super simple (minimal, noncontroversial) GitHub Action to enable further experimentation with Actions such as #3386 and #3390. This PR is similar to #3276 and #3313 but running on GitHub Actions instead of Travis CI.

@tysonite @hugovk Your reviews please.

[tysonite]

Looks good to me with some questions.

[tysonite]

Would you like to run it on pull request as well?

[cclauss]

Everyone pushes but insiders do not pull request.

[tysonite]

Is there any reason only E9 and F63 are selected?

[cclauss]

https://flake8.pycqa.org/en/latest/user/error-codes.html

On the flake8 test selection, this PR does not focus on "style violations" (the majority of flake8 error codes that psf/black can autocorrect). Instead these tests are focus on runtime safety and correctness:

• E9 tests are about Python syntax errors usually raised because flake8 can not build an Abstract Syntax Tree (AST). Often these issues are a sign of unused code or code that has not been ported to Python 3. These would be compile-time errors in a compiled language but in a dynamic language like Python they result in the script halting/crashing on the user.
• F63 tests are usually about the confusion between identity and equality in Python. Use ==/!= to compare str, bytes, and int literals is the classic case. These are areas where a == b is True but a is b is False (or vice versa). Python >= 3.8 will raise SyntaxWarnings on these instances.
• F7 tests logic errors and syntax errors in type hints
• F82 tests are almost always undefined names which are usually a sign of a typo, missing imports, or code that has not been ported to Python 3. These also would be compile-time errors in a compiled language but in Python a NameError is raised which will halt/crash the script on the user.

[tysonite]

Thanks. Understood.

[cclauss]

Tests passing.

[pekkaklarck]

Linting is something that needs to be discussed separately. I'm pretty careful with formatting when committing code (and reviewing PRs) and while I most of the time follow PEP-8 and other good conventions, there are cases where that isn't possible or results with worse code (at least in my opinion). I really dislike the idea to add magic comments to tell linters to ignore errors in such cases. Should at least check how many errors/warnings we get and would there be configs that could allow disabling offending validations for good.

Another possibility would be using black and letting it do all the formatting. I don't agree with all of its formatting choices but I guess I could get used to it. Having such an automatic formatter would ease PR review so much that it looks really tempting.

[hugovk]

Yes, Black is really good. I also don't agree with everything but it's so much easier to just go along with it and do away with those trivial arguments :)

[cclauss]

I will open a separate PR to add autoblack which will enforce Black formatting only on the files that change vs. master. This allows for gradual migration to Black-compliant formatting.

[pekkaklarck]

If we take black to use, we should run it through the whole code base to keep future diffs small. I just quickly checked that black --skip-string-normalization --diff src/robot doesn't look too bad. I'm not yet ready to make the change but will think about it.

[pekkaklarck]

This black feature/bug is pretty annoying: psf/black#619

[adrianyorke]

+1 for Black formatting. We have been using that on our internal projects for a while now. The conversations about formatting have all but ended. We have gone for pure Black with no overrides so we have even accepted the slightly odd "double-quotes everywhere" format. A few styles will seem odd at first and you will instinctively want to override certain settings which of course works against format standardization. The question should be: "Can you live with it?".

[cclauss]

This black feature/bug is pretty annoying: psf/black#619

Yawn... Get over it and move on. It is way more important to have one common formatting across all Python projects then it is to worry about these extra blank lines.

[cclauss]

@Hi-Fi Your review please.

[Hi-Fi]

I think linting checks in CI are good, as long as results are also accepted by developers. So this looks OK to me. I just prefer squashing commits before PR merge to keep history clearer.

[pekkaklarck]

I'm not at all certain linters on CI would help me. Let's concentrate on getting acceptance tests running on CI first. After that (and after RF 3.2) we can discuss this, and Black, more.

[cclauss]

I thought you delegated to @Hi-Fi ... There are lint issues to fix and we should stop sweeping them under the carpet.

[cclauss]

@Hi-Fi squashed commits as requested.

[pekkaklarck]

I delegated infrastructure work and decisions to @Hi-Fi but I still plan to make decisions about what features we actually are going to use because they affect my day-to-day work. Adding CI that I will just ignore wouldn't benefit anyone.

At the moment the biggest problem is time. With RoboCon 2020 organzation, RF 3.2 (already late), and trying to get acceptance tests running on different platforms, I simply won't have time to look at linting problems.

I also doubt our code base has lot of severe style related issues and would be really surprised if there were problems that could cause actual functional bugs. If you find something like that, please submit separate issues and I promise to look at them. Style related linting problems, and possibly using Black, are something we can discuss once we got acceptance test CI ready, RF 3.2 close to the release candidate phase, and RoboCon behind. These would actually be good topics to discuss and possibly work on during RoboCon sprints.

[cclauss]

this PR does not focus on "style violations" Instead these tests are focus on runtime safety and correctness.