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

Omit repeated equality comparison for sys #10054

Merged

Conversation

arjunnn
Copy link
Contributor

@arjunnn arjunnn commented Feb 19, 2024

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

{
let excluder = Regex::new(r"^(platform|version)").unwrap();
return !excluder.is_match(left.clone().expect_attribute_expr().attr.as_str());
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Can you try something like:

    any_over_expr(test, &|expr| {
        semantic.resolve_call_path(expr).is_some_and(|call_path| {
            matches!(call_path.as_slice(), ["sys", "version_info" | "platform"])
        })
    })

This is roughly taken from a helper we have called is_sys_version_block.

Copy link
Member

@charliermarsh charliermarsh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just putting this back in your queue :)

@charliermarsh charliermarsh enabled auto-merge (squash) February 20, 2024 18:56
@charliermarsh charliermarsh added the bug Something isn't working label Feb 20, 2024
@charliermarsh charliermarsh merged commit 175c266 into astral-sh:main Feb 20, 2024
16 checks passed
Copy link

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

@arjunnn
Copy link
Contributor Author

arjunnn commented Feb 20, 2024

Sorry, I couldn't get to this in time. Thank you!

@charliermarsh
Copy link
Member

No prob, I was just in PR-closing-mode and took advantage of being in the area. Thanks for contributing!

nkxxll pushed a commit to nkxxll/ruff that referenced this pull request Mar 10, 2024
## 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
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PLR1714 fixing sometimes breaks mypy checks
2 participants