Skip to content
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

GitHub Action to lint Python code #6227

Closed
wants to merge 5 commits into from
Closed

Conversation

cclauss
Copy link
Contributor

@cclauss cclauss commented Dec 31, 2020

Summary: Add a GitHub Action to lint Python code

A superset of #2487

Output: https://github.com/cclauss/discord.py/actions

Checklist

  • If code changes were made then they have been tested.
    • I have updated the documentation to reflect the changes.
  • This PR fixes an issue.
  • This PR adds something new (e.g. new method or parameters).
  • This PR is a breaking change (e.g. methods or parameters removed/renamed)
  • This PR is not a code change (e.g. documentation, README, ...)

@cclauss cclauss mentioned this pull request Dec 31, 2020
4 tasks
@Rapptz
Copy link
Owner

Rapptz commented Dec 31, 2020

I'm not overly a fan of autolinting nor do I personally use linters due to a lot of false positives and leading to superfluous contributions (not this one, to make it clear). I don't want to litter my code with comments that tell a linter to stop doing something since I consider it noisy. Likewise, currently my account is restricted and can't use GitHub Actions. I'll probably get that bit sorted in a few since I keep putting it off.

I appreciate the PR but as it stands at the moment I need to think about it.

@cclauss
Copy link
Contributor Author

cclauss commented Dec 31, 2020

I don't want to litter my code with comments that tell a linter to stop doing something since I consider it noisy.

Agreed. That is why I use flake8 . --select=E9,F63,F7,F82 to focus the linter only on a few essential issues that the compiler would complain about in a compiled language. With this focused subset, no liter directives were needed but the linter did highlight code that would crash on the user if run.

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.

@Rapptz
Copy link
Owner

Rapptz commented Dec 31, 2020

Right now the workflow executes a few programs like black/bandit/codespell/etc that I assume you kept there since this is a template of sorts. Right now I think that's okay and not something I'm hung up about but I just wanted to make a note of it because at the initial read I had on this PR I saw a lot of superfluous warnings come from bandit in particular which led me to believe I would be dealing with more false positives than I expected. However I'm seeing now that these are essentially just logged and ignored since they go through || true.

On the topic of the selected lints in particular:

  • E9 is fine, since that's just a SyntaxError.
  • F63 is fine if it's only bound to those specific types. The library uses is for things such as identity relation for references or sentinels or tribools.
  • F7 seems a bit vague. Right now the library uses type hints as an API contract between the user which allows usage of typehints that are not strictly PEP-compliant though they mostly are, for example having instances for state tracking or passing functions is well defined. However none of this currently affects the library code since it isn't type hinted at the moment.
  • F82 is a good one. Though it seems that it caught a stub in the example which was a placeholder. The current PR assumes these are strings but they're not, they're PartialEmoji objects.

I'm fine with the PR, these are just my thoughts on the matter.

`global partial_emoji_1, partial_emoji_2`
@cclauss cclauss closed this Feb 5, 2021
@cclauss cclauss deleted the patch-4 branch February 5, 2021 17:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants