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

Use flake8-pie #6164

Merged
merged 9 commits into from
Sep 17, 2022
Merged

Conversation

AlpAribal
Copy link
Contributor

@AlpAribal AlpAribal commented Aug 14, 2022

Relates-to: #4776

Adding the hook leads to four new warnings for the repo. Each warning was handled in a separate commit.

  • PIE781: assign-and-return
    All instances of this warning were fixed [Edit: This warning is ignored altogether as it does not play well with mypy and there is not 100% agreement with the enforced style.]
  • PIE803: 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: precise-exception-handlers
    PR ignores instances where the intention is indeed to catch any exceptions and fixes the rest.
  • PIE798: no-unnecessary-class
    All instances are ignored via an additional entry in flake8 config.

I am aware that some ignore/fix decisions can be considered opinionated, and am happy to make changes where necessary.

@AlpAribal AlpAribal marked this pull request as draft August 14, 2022 12:46
@dimbleby
Copy link
Contributor

as the pipelines are showing you, pie781 is at odds with appeasing mypy. IMO it's more important that mypy be happy.

(I believe that there's work going on to improve the typing eg in cleo, so perhaps we'll one day be able to avoid the mypy-appeasement and just write the code more naturally)

@AlpAribal
Copy link
Contributor Author

as the pipelines are showing you, pie781 is at odds with appeasing mypy. IMO it's more important that mypy be happy.

How would you recommend tackling this issue? Should I ignore pie781 for good or ignore mypy type warnings for these cases?

@dimbleby
Copy link
Contributor

How would you recommend tackling this issue?

I don't much mind either way. The main thing is that I think that the mypy checking is valuable, so making that happy should 'win'.

Personally I rather like the style that pie781 objects to so I'd probably suppress it altogether, but this is certainly a matter of taste.

If you think it's generally a valuable warning then I guess ignore it explicitly in the places where we have deliberately gone against it for the purpose of typechecking.

@AlpAribal
Copy link
Contributor Author

Personally I rather like the style that pie781 objects to so I'd probably suppress it altogether, but this is certainly a matter of taste.

I see, I do not have a strong preference either. I will ignore the warning.

@AlpAribal AlpAribal marked this pull request as ready for review August 14, 2022 17:53
@neersighted neersighted merged commit c1aff5c into python-poetry:master Sep 17, 2022
@neersighted neersighted added this to the 1.3 milestone Sep 17, 2022
@neersighted neersighted added kind/enhancement Not a bug or feature, but improves usability or performance area/ci Related to CI/CD labels Sep 17, 2022
Copy link

This pull request 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 Feb 29, 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 kind/enhancement Not a bug or feature, but improves usability or performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants