Skip to content

adopt flake8-tidy-imports to enforce O.1.5 #223

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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

mshafer-NI
Copy link
Collaborator

Reason

We have O.1.5 which says:

DO Use absolute imports

We don't have enforcement of this.

If a non-absolute import is used, our current tooling (fix and format) has different behavior depending on if it is run above the module (in the folder with pyproject.toml) or in a sub-folder within the module. This is (in part) caused by us setting the app-import-names` automatically if the user does not specify it. (and we don't include neighbor sub-modules).

This PR is a way to address
#215

Adopting flake8-tidy-imports + explicitly setting it to ban "all relative" imports, the case mentioned in that issue is dealt with by flagging that "neighbor" import with I252.

Pros:

Simple, opinionated rule

Cons:

(this example assumes a layout similar to):

module
|   fizz.py
|   __init__.py
|
\---foo
        __init__.py

As PEP328 defines it, if the import "import foo" is present, it is considered to be an absolute import (even though the intention is to resolve to a neighboring sub-module); while, "import .foo" or "from . import foo" are both relative imports.
As flake8-tidy-imports is currently implemented, this would also ban an "absolute" import that is meant to resolve to a neighbor.

Rationale:

  • Using "import foo" in fizz.py is ambiguous for human readers. Do we want module.foo? or should foo be coming from an install-time dep?
    • If we had further caused confusion by having (e.g.) a subprocess sub-module, the code import subprocess would resolve to this module's subprocess even if it was intended to resolve to stdlib. By adding this rule in, that would now be required to be import module.subprocess or from module import subprocess.
  • Requiring imports be absolute to the root module (module) makes it clear that this is "first-party" and removes ambiguity over whether iSort is doing the right thing or not.

@mshafer-NI mshafer-NI requested review from irwand and neilvana as code owners May 7, 2025 17:02
Copy link
Contributor

github-actions bot commented May 7, 2025

Thank you for contributing! 👋

@mshafer-NI
Copy link
Collaborator Author

The review-bot was supposed to tag the NI engineers for comment due to this changing the Conventions doc:
@alejandro5042
@DavidCurtiss
@irwand
@jryckman
@kcuzner
@mshafer-NI
@neilvana
@nethrasuresh
@pakdev
@sbethur
@stick152
@tkrebes
@Adithyak1998
@innagarc
@ShibaniRout

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.

1 participant