search

linode / linode_api4-python

Official Python bindings for the Linode API
https://linode-api4.readthedocs.io/en/latest/
BSD 3-Clause "New" or "Revised" License
135 stars 75 forks source link

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

Closed zliang-akamai closed 3 months ago

zliang-akamai commented 3 months ago

📝 Description

To cleanup unused imports

✔️ How to Test

make testunit
lgarber-akamai commented 3 months ago

@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 commented 3 months ago

@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 commented 3 months ago

@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 👍