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

Code Quality: Additional lints in this repository (help wanted!) #4776

Closed
neersighted opened this issue Nov 16, 2021 · 28 comments · Fixed by #4985
Closed

Code Quality: Additional lints in this repository (help wanted!) #4776

neersighted opened this issue Nov 16, 2021 · 28 comments · Fixed by #4985
Labels
area/ci Related to CI/CD good first issue kind/enhancement Not a bug or feature, but improves usability or performance

Comments

@neersighted
Copy link
Member

neersighted commented Nov 16, 2021

We're doing a bunch of code quality cleanup, and as part of that effort I'd like to implement some additional lints/coding conventions. There will be a lot of manual changes here, so I think these will be some items to have multiple people tackle.

In no particular order:

  • Add ban-relative-imports = true to .flake8 and remove relative imports from the repo.
  • Add flake8-type-checking==1.0.4 to .pre-commit-config.yaml with enable-extensions = TC, TC2 in .flake8, and rework our type annotations for zero runtime cost using string constants. When we drop Python 3.6 support, pyupgrade will be used to automatically switch to from __future__ import annotations and unquote the annotations.
  • Add flake8-use-fstring==1.3 to .pre-commit-config.yaml and fix all the use of .format() on string constants in the repo. Start at level 0, and try level 1 if possible. Level 2 is likely useless in this repo (I think we will always use .format() on string variables), but you could prove me wrong.
  • Remove E501 from .flake8 -- currently, black leaves our long comments, docstrings, and string constants alone -- let's hand-format them to comply with the line length limit.

Please call dibs in this issue to prevent duplicated effort.

(suggestions for additional code quality improvements/tools to enforce them are also welcomed here)

@neersighted neersighted added Good First Issue area/ci Related to CI/CD kind/enhancement Not a bug or feature, but improves usability or performance labels Nov 16, 2021
@neersighted
Copy link
Member Author

neersighted commented Nov 16, 2021

Here are some other tools I have glanced at, but not yet fully investigated for usefulness (of the tool itself, or the change):

PRs adding these tools are welcome -- even if you just add it and don't fix any lints (so we can see the magnitude of correction required). We can then discuss the relative merits of these additional tools on that PR.

@neersighted
Copy link
Member Author

neersighted commented Nov 16, 2021

@branchvincent
Copy link
Member

  • Add ban-relative-imports = true to .flake8 and remove relative imports from the repo.

happy to take this one!

@danieleades
Copy link
Contributor

danieleades commented Nov 16, 2021

i'll take pep8-naming

see #4778

@wagnerluis1982
Copy link
Contributor

Taking the occasion, I'd like to discuss the following, in the hope of be handled as part of this effort:

poetry-schema.json: I noticed that both projects (poetry and poetry-core) carry a schema file. Is that really required? I guess we could have only one file in poetry-core and be consumed by poetry when/if required.

@abn
Copy link
Member

abn commented Nov 16, 2021

@wagnerluis1982 I have moved that to #4780

@branchvincent
Copy link
Member

  • Add flake8-use-fstring==1.3

I'll take this one as well

@branchvincent
Copy link
Member

Added flake8-use-fstring to poetry-core. Before adding here, we prob want to merge #4758?

@danieleades
Copy link
Contributor

Are any of these plugins redundant? For example, does it make sense to use both pyupgrade and use-f-strings?

@neersighted
Copy link
Member Author

Are any of these plugins redundant? For example, does it make sense to use both pyupgrade and use-f-strings?

pyupgrade does not enforce f-strings exactly -- it's only clever enough to do maybe 40% of the job (check out my PR). As far as the ones I have not researched/trialed in detail, they could be -- we'll have to discuss that on the individual PRs (or someone can do some research into actual usefulness and post here if they want to, I suppose).

@radoering
Copy link
Member

* Add `flake8-type-checking==1.0.4`

I'll give this one a try.

@finswimmer
Copy link
Member

finswimmer commented Nov 17, 2021

Hello @radoering,

I played around with it locally and it worked quite well. The only thing I dislike is, that it forces one to put imports from typing, like List or Dict, into the type-checking block and so they need to be quoted. This looks quite ugly.

I would suggest keeping these imports outside the type-checking block and mark them with # noqa: TC002.

@radoering
Copy link
Member

I would suggest keeping these imports outside the type-checking block and mark them with # noqa: TC002.

@finswimmer: Easier said than done 😉: snok/flake8-type-checking#49 Fortunately, the maintainer released a new version with my patch as quick as greased lightning. He even made an offer to "add a setting to allow specific module imports - like a whitelist/passlist for modules exempt from checking". For now, I'll add the # noqa: TC002. Maybe, you can comment on the linked issue if you think that poetry will benefit from such a setting.

@neersighted
Copy link
Member Author

neersighted commented Nov 17, 2021

@radoering @finswimmer https://pypi.org/project/future-annotations/ is available as a third option. I'm inclined to just live with strings for now because we can remove them in a month or two when we drop 3.6 support, but if you really want to, there is an option to use the future syntax now.

Edit: just saw the linked discussion -- awesome! Looks like we'll get to have our cake and eat it too!

@finswimmer
Copy link
Member

@neersighted
Copy link
Member Author

Here are two more flake8-plugins I find useful:

* https://github.com/adamchainz/flake8-comprehensions
...

That one is actually already implemented! It's pretty useful, yeah.

@neersighted
Copy link
Member Author

Added a new item to the list of 'wanted' lints:

Remove E501 from .flake8 -- currently, black leaves our long comments, docstrings, and string constants alone -- let's hand-format them to comply with the line length limit.

@radoering
Copy link
Member

I'll have a look at flake8-simplify in the next few days. (Please have a look at the PR for a first question.)

@stinodego
Copy link
Contributor

I'll gladly pick up the removal of E501 and fix some line lengths. Expect a PR tomorrow :)

@finswimmer
Copy link
Member

I learned about nitpick these days. I wonder if we like to use it, to enforce a minimum set of checks across our repos?

@Secrus
Copy link
Member

Secrus commented Apr 6, 2022

is this still active? are tools that are not crossed out still to be implemented?

@finswimmer
Copy link
Member

Hey @Secrus,

I would say the tools that are not crossed are debatable, whether we really want to use or not. The only one I really want to see is flake8-functions. But I guess this is not an easy one, as it would require some refactoring. If you like to pick this one, I would suggest starting applying it to poetry-core and split the PR's into chunks that can be reviewed easily.

fin swimmer

@dogweather
Copy link

Hey @finswimmer,

The only one I really want to see is flake8-functions. But I guess this is not an easy one, as it would require some refactoring. If you like to pick this one, I would suggest starting applying it to poetry-core and split the PR's into chunks that can be reviewed easily.

I'd like to give this a shot. I saw that flake8-function checks if a function is not pure which would be amazing. I didn't know that such a linter existed for Python.

@Secrus
Copy link
Member

Secrus commented Jul 1, 2022

Hey @finswimmer,

I'd like to give this a shot. I saw that flake8-function checks if a function is not pure which would be amazing. I didn't know that such a linter existed for Python.

@dogweather I tried flake8-functions myself, and have few conclusions:

  • „purity” checks are probably broken. In both poetry and poetry-core I got 0 functions reported as „not pure”.
  • Adding this would require a lot of refactors (function lengths and amount of arguments of the function), so it would probably need to be multi-PR job, going module by module with incremental addoption.
  • With how much refactors there would be, I believe this is not something we want to touch before version 1.2 is released. Post-1.2 is planned as refactoring time, so I would hołd on with the changes until then.

@dimbleby
Copy link
Contributor

dimbleby commented Jul 4, 2022

I find quite a lot to like in flake8-pie.

Not sure I agree with absolutely everything it warns about, but you can always disable the things that you don't want.

(I don't plan to work on adding this to the repository, if anyone wants to have a go then we will not be duplicating one another.)

@AlpAribal
Copy link
Contributor

I would like to add flake8-pie. Can make a PR today or tomorrow.

neersighted pushed a commit that referenced this issue Sep 17, 2022
Relates-to: #4776 

Adding the hook leads to four new warnings for the repo.
-
[PIE803](https://github.com/sbdchd/flake8-pie#pie803-prefer-logging-interpolation):
prefer-logging-interpolation
This check produces false positives (`debug()` calls are always flagged
as if they belong to a logger). PR ignores such instances and fixes the
rest.
-
[PIE786](https://github.com/sbdchd/flake8-pie#pie786-precise-exception-handlers):
precise-exception-handlers
PR ignores instances where the intention is indeed to catch any
exceptions and fixes the rest.
-
[PIE798](https://github.com/sbdchd/flake8-pie#pie798-no-unnecessary-class):
no-unnecessary-class
All instances are ignored via an additional entry in flake8 config.
@neersighted
Copy link
Member Author

I'm going to close this for now as the original effort discussed here has largely concluded over the last year. However, please do suggest new linters/code quality tools if you think they may bring improvements to the project's code, or head off common footguns.

Copy link

github-actions bot commented Mar 1, 2024

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/ci Related to CI/CD good first issue kind/enhancement Not a bug or feature, but improves usability or performance
Projects
None yet
Development

Successfully merging a pull request may close this issue.