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

Update DeprecatedABCImport Rule #474

Merged
merged 6 commits into from
Jul 29, 2024

Conversation

surge119
Copy link
Contributor

@surge119 surge119 commented Jul 25, 2024

Summary

In the instance of there being a try except that falls back to the deprecated import, this rule would attempt to change the fallback. An example:

try:
    from collections.abc import Container
except ImportError:
    from collections import Container

This above example should be valid, but was previously not. Previously, the lint rule would attempt to fix the import in the except block

Test Plan

make

@surge119 surge119 requested review from amyreese and zsol as code owners July 25, 2024 21:26
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 25, 2024
@surge119 surge119 changed the title Update deprecatedabcimport rule Update DeprecatedABCImport Rule Jul 25, 2024
Copy link
Member

@amyreese amyreese left a comment

Choose a reason for hiding this comment

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

I think it would be better/faster to use ParentNodeProvider metadata to see if an import is directly nested inside a try/except block, rather than walking every try/except in the entire file regardless of whether it may or may not contain any imports. I think the code would also be simpler that way, and wouldn't require caching a set of ignored nodes.

@surge119 surge119 requested a review from amyreese July 29, 2024 20:33
Copy link
Member

@amyreese amyreese left a comment

Choose a reason for hiding this comment

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

Less code 👍 👍

@amyreese amyreese merged commit 48491b4 into Instagram:main Jul 29, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants