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.

Sign up for GitHub

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

Jump to bottom

PLR1714 fixing sometimes breaks mypy checks #10017

Closed
RonnyPfannschmidt opened this issue Feb 17, 2024 · 1 comment · Fixed by #10054
Closed

PLR1714 fixing sometimes breaks mypy checks #10017

RonnyPfannschmidt opened this issue Feb 17, 2024 · 1 comment · Fixed by #10054
Labels
bug Something isn't working good first issue Good for newcomers rule Implementing or modifying a lint rule

Comments

@RonnyPfannschmidt
Copy link

RonnyPfannschmidt commented Feb 17, 2024 •
edited
Loading

as per pytest-dev/pytest#11998

while i presume that most free-form uses should apply,
mypy cannot identify set membership checks for certain usages

so sys.platform in {"win32", "emscripten"} would trigger mypy confusion while sys.platform == "win32" or sys.platform == "emscripten" would ensure mypy picks the correct stubs for each condition

a first naive heuristics could ensure the rule wont be apply to members of sys/platform
@charliermarsh
Copy link
Member

That’s a good idea. We should omit sys platform and Python version checks here.

@charliermarsh charliermarsh added bug Something isn't working good first issue Good for newcomers labels Feb 17, 2024
@AlexWaygood AlexWaygood added the rule Implementing or modifying a lint rule label Feb 18, 2024
@arjunnn arjunnn mentioned this issue Feb 19, 2024
Merged
charliermarsh pushed a commit that referenced this issue Feb 20, 2024
@arjunnn
Omit repeated equality comparison for sys (#10054)
175c266 
## Summary
Update PLR1714 to ignore `sys.platform` and `sys.version` checks. 
I'm not sure if these checks or if we need to add more. Please advise.

Fixes #10017

## Test Plan
Added a new test case and ran `cargo nextest run`
nkxxll pushed a commit to nkxxll/ruff that referenced this issue Mar 10, 2024
@arjunnn @nkxxll
Omit repeated equality comparison for sys (astral-sh#10054)
92e3445 
## Summary
Update PLR1714 to ignore `sys.platform` and `sys.version` checks. 
I'm not sure if these checks or if we need to add more. Please advise.

Fixes astral-sh#10017

## Test Plan
Added a new test case and ran `cargo nextest run`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working good first issue Good for newcomers rule Implementing or modifying a lint rule
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Omit repeated equality comparison for sys arjunnn/ruff
3 participants
@RonnyPfannschmidt @charliermarsh @AlexWaygood