Skip to content

Turn on remove-all-unused-imports in for autoflake - #414

Merged
zliang-akamai merged 1 commit into
linode:devfrom
zliang-akamai:zhiwei/turn-on-remove-all-unused-imports
Jun 11, 2024
Merged

Turn on remove-all-unused-imports in for autoflake#414
zliang-akamai merged 1 commit into
linode:devfrom
zliang-akamai:zhiwei/turn-on-remove-all-unused-imports

Conversation

@zliang-akamai

Copy link
Copy Markdown
Member

📝 Description

To cleanup unused imports

✔️ How to Test

make testunit

@zliang-akamai
zliang-akamai requested a review from a team as a code owner June 6, 2024 21:53
@zliang-akamai
zliang-akamai requested review from jriddle-linode and lgarber-akamai and removed request for a team June 6, 2024 21:53
@lgarber-akamai

Copy link
Copy Markdown
Contributor

@zliang-akamai This PR looks great!

I do have one concern: given this is a package intended to be consumed by users would this accidentally break any indirect imports? Most of the dropped imports seem pretty insignificant but it might be worth double-checking just in case 🙂

@zliang-akamai

zliang-akamai commented Jun 7, 2024

Copy link
Copy Markdown
Member Author

@lgarber-akamai Thanks for the reminder!

I think all imports it removed is not relevant to the module it's in.

For example, from linode_api4.objects.profile import PersonalAccessToken is removed in linode_api4/groups/account.py.

The right way to import PersonalAccessToken is from linode_api4.objects.profile import PersonalAccessToken rather than something like from linode_api4.groups.account import PersonalAccessToken.

I doubt there is any indirect import like that, and even that's the case, they can easily fix it by changing the import path.

Autoflake would ignore (not to remove) any unused import in __init__.py, so the importing from a module won't be impacted. For example, from linode_api4 import DomainRecord is not impacted, and you don't have to import it via absolute path, like from linode_api4.objects.domain import DomainRecord.

But we can definitely mark it as a breaking change as a head up to the users.

@lgarber-akamai

Copy link
Copy Markdown
Contributor

@lgarber-akamai Thanks for the reminder!

I think all imports it removed is not relevant to the module it's in.

For example, from linode_api4.objects.profile import PersonalAccessToken is removed in linode_api4/groups/account.py.

The right way to import PersonalAccessToken is from linode_api4.objects.profile import PersonalAccessToken rather than something like from linode_api4.groups.account import PersonalAccessToken.

I doubt there is any indirect import like that, and even that's the case, they can easily fix it by changing the import path.

Autoflake would ignore (not to remove) any unused import in __init__.py, so the importing from a module won't be impacted. For example, from linode_api4 import DomainRecord is not impacted, and you don't have to import it via absolute path, like from linode_api4.objects.domain import DomainRecord.

But we can definitely mark it as a breaking change as a head up to the users.

Sounds good to me! I think this is a very positive change, we just need to make sure we communicate it well 👍

@jriddle-linode jriddle-linode left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM +1 to communication.

@zliang-akamai zliang-akamai added the breaking-change for breaking changes in the changelog. label Jun 11, 2024
@zliang-akamai
zliang-akamai merged commit bee13ce into linode:dev Jun 11, 2024
@zliang-akamai
zliang-akamai deleted the zhiwei/turn-on-remove-all-unused-imports branch June 11, 2024 15:12
@lgarber-akamai lgarber-akamai mentioned this pull request Jun 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking-change for breaking changes in the changelog.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants