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

Ignore F401 for __init__.py files #173

Merged
merged 2 commits into from
May 2, 2024
Merged

Conversation

mbr0wn
Copy link
Contributor

@mbr0wn mbr0wn commented Apr 4, 2024

These files routinely import other modules without immediately using them with the intention of making them available to the importing module. Moreover, they are rarely even supposed to be used within the same __init__.py file. Marking all of them 'noqa: F410' is redundant.

Copy link
Contributor

github-actions bot commented Apr 4, 2024

Thank you for contributing! 👋

Copy link
Contributor

github-actions bot commented Apr 4, 2024

Thank you for contributing! 👋

Since this changes the Coding Conventions, I'll @-mention the appropriate NI engineers.
@alejandro5042
@DavidCurtiss
@irwand
@jryckman
@kcuzner
@mshafer-NI
@neilvana
@nethrasuresh
@pakdev
@rtzoeller
@sbethur
@stick152
@tkrebes
@Adithyak1998
@innagarc
@ShibaniRout

@@ -858,13 +858,21 @@ import cheese_shop.brie

### [O.1.8] ❌ **DO NOT** Import definitions that are not used 💻

> 💻 This rule is enforced by error code F401
> 💻 This rule is enforced by error code F401, except in `__init__.py` files
Copy link
Collaborator

@mshafer-NI mshafer-NI Apr 4, 2024

Choose a reason for hiding this comment

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

Have you considered declaring the __all__ list? this creates a reference while also explicitly declaring the API.
(reference https://docs.python.org/3/tutorial/modules.html#importing-from-a-package)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @mshafer-NI ,

I don't understand the connection to this change request. I'm talking about not having to amend files like this: https://github.com/ni/python-styleguide/blob/main/ni_python_styleguide/_utils/__init__.py

If I understand this correctly, __all__ is for people doing from x import * (which they shouldn't do according to our own style guide). This doesn't apply to existing __init__.py files that already import stuff from submodules.

Copy link
Collaborator

Choose a reason for hiding this comment

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

__all__ is set in the __init__ file (and yes, it's impact is on clients doing from from x import *)

I'll send you an example offline

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mshafer-NI thanks for sending those explanations, and also the docs on __all__, I learned something along the way.

After processing it for a few days, I still think this here is the right way to go. Also, having scoured peoples thoughts and opinions I really think that __all__ should be used when from X import * does something undesirable, like export functions we want otherwise not exported, or trigger some action that takes long. It should not be used just so we can disable F401 warnings without having to type noqa: F401. The downside of using __all__ is that you now have to maintain lists of exports when doing other changes, which increases maintenance burden and redundancy in __init__.py files.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for the response. Two notes:

  1. This is a drastic departure from current direction and requires more changes in this PR (this rule becomes an "Avoid" instead of "Do Not", and it needs a lot more examples of when is ok and when is not)
  2. We will need buy-in from the rest of the PWG members. I've added this to the agenda for our next meeting on April 17th and will forward you the invite.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The suggestion I'm making is extremely narrow (just limited to __init__.py files), so we should keep the "Do Not" (but I guess we need to update the title). Everything that was previously done to avoid this error (use noqa or duplicate symbol names into __all__) is still valid and won't throw errors/warnings. I think this won't break the bank 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the language according to our conversation.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you for your patience with this.

@mbr0wn mbr0wn force-pushed the mbr0wn/add-f401 branch 2 times, most recently from 188de88 to 0ebaf5f Compare April 17, 2024 14:47
@mbr0wn mbr0wn requested a review from mshafer-NI April 17, 2024 14:48
@mshafer-NI mshafer-NI requested review from a team and removed request for a team April 17, 2024 15:05
These files routinely import other modules without immediately using
them with the intention of making them available to the importing
module. Moreover, they are rarely even supposed to be used within the
same __init__.py file. Marking all of them 'noqa: F410' is redundant.
@mshafer-NI mshafer-NI merged commit 45df40c into ni:main May 2, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants