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

Select stable import name when multiple possible bindings are in scope #12888

Merged
merged 1 commit into from
Aug 16, 2024

Conversation

MichaReiser
Copy link
Member

Summary

Fixes #12885

The issue with the FURB177 test is that there are two viable bindings for pathlib.Path: pathlib.Path or Path but bindings is a HashMap that has no guaranteed iteration order.

I'm reluctant to change bindings to a BTreeMap. I would suspect that it comes with quiet a performance penalty. That's why this PR fixes this issue locally
by sorting the imports by their range and picking the last import if multiple options exist.

Test Plan

cargo test

Alternatives

Split the test into two files. One that uses import pathlib and one with from pathlib import Path

@MichaReiser MichaReiser added the internal An internal refactor or improvement label Aug 14, 2024
@MichaReiser MichaReiser changed the title Select stable import name Select stable import name when multiple possible bindings are in scope Aug 14, 2024
Copy link
Contributor

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@WhyNotHugo
Copy link
Contributor

Tests pass on 32bit architectures: https://gitlab.alpinelinux.org/WhyNotHugo/aports/-/pipelines/252939

Some of the more exotic architectures are not done, but this does fix #12885

algitbot pushed a commit to alpinelinux/aports that referenced this pull request Aug 15, 2024
@MichaReiser
Copy link
Member Author

@charliermarsh any thoughts on this?

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.

Thanks!

@MichaReiser MichaReiser merged commit d61d75d into main Aug 16, 2024
20 checks passed
@MichaReiser MichaReiser deleted the stable-import-fix branch August 16, 2024 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal An internal refactor or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

0.5.7 tests fail on 32bit systems
3 participants