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

Fix issues with SIM101 (adjacent isinstance() calls) #7798

Merged
merged 6 commits into from
Oct 4, 2023

Conversation

JelleZijlstra
Copy link
Contributor

  • Only trigger for immediately adjacent isinstance() calls with the same target
  • Preserve order of or conditions

Two existing tests changed:

  • One was incorrectly reordering the or conditions, and is now correct.
  • Another was combining two non-adjacent isinstance() calls. It's safe enough in that example,
    but this isn't safe to do in general, and it feels low-value to come up with a heuristic for
    when it is safe, so it seems better to not combine the calls in that case.

Fixes #7797

- Only trigger for immediately adjacent isinstance() calls with the same target
- Preserve order of or conditions

Two existing tests changed:
- One was incorrectly reordering the or conditions, and is now correct.
- Another was combining two non-adjacent isinstance() calls. It's safe enough in that example,
  but this isn't safe to do in general, and it feels low-value to come up with a heuristic for
  when it is safe, so it seems better to not combine the calls in that case.

Fixes astral-sh#7797
@charliermarsh charliermarsh added the bug Something isn't working label Oct 4, 2023
@charliermarsh charliermarsh enabled auto-merge (squash) October 4, 2023 04:26
@charliermarsh
Copy link
Member

Looks great, thanks @JelleZijlstra!

auto-merge was automatically disabled October 4, 2023 04:33

Head branch was pushed to by a user without write access

@charliermarsh charliermarsh enabled auto-merge (squash) October 4, 2023 04:35
@charliermarsh charliermarsh merged commit 7b4fb4f into astral-sh:main Oct 4, 2023
14 checks passed
@codspeed-hq
Copy link

codspeed-hq bot commented Oct 4, 2023

CodSpeed Performance Report

Merging #7798 will improve performances by 2.42%

Comparing JelleZijlstra:sim101 (3cea5ec) with main (5d49d26)

Summary

⚡ 2 improvements
✅ 23 untouched benchmarks

Benchmarks breakdown

Benchmark main JelleZijlstra:sim101 Change
linter/default-rules[pydantic/types.py] 38.9 ms 38 ms +2.42%
linter/all-rules[numpy/ctypeslib.py] 35.4 ms 34.7 ms +2.17%

@github-actions
Copy link
Contributor

github-actions bot commented Oct 4, 2023

PR Check Results

Ecosystem

✅ ecosystem check detected no changes.

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.

SIM101 fix reverses order of or condition
2 participants