Content-Length: 266273 | pFad | http://github.com/astral-sh/ruff/pull/16772

70 [`flake8-tidy-imports`] Implement `relative-sibling-imports` (`TID254`) by noirbizarre · Pull Request #16772 · astral-sh/ruff · GitHub
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

[flake8-tidy-imports] Implement relative-sibling-imports (TID254) #16772

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

noirbizarre
Copy link

Summary

This PR follows the discussion from #1014 and add a new opt-in rule TID254 which is requiring relative import for sibling (and nested) modules.
It requires the linter.flake8-tidy-imports.relative-sibling-imports setting to be true to avoid users having enabled all TID to have their imports rewritten if they did not choose to as it is one of 2 suggested PEP8 alternatives:

However, explicit relative imports are an acceptable alternative to absolute imports, especially when dealing with complex package layouts where using absolute imports would be unnecessarily verbose:

from . import sibling
from .sibling import example

Fixes #1014

Test Plan

Multiple test cases have been added to the test suite, with associated expected snapshots, and it has been tested on some external codebases.

@noirbizarre noirbizarre force-pushed the feature/tid254-relative-sibling-imports branch from 1c7fb2c to ba055a0 Compare March 16, 2025 00:55
@MichaReiser
Copy link
Member

Thanks for working on this. It might take me a while before I get to thinking through the implications of this change. The concerns are:

  • It creates a new rule that doesn't exist upstream. We've made exceptions for some categories in the past but it comes with its own set of problems, which is why we're more hesitant to keep doing that (e.g. what if upstream adds a TID254...)
  • Rules that require explicit opt-in with an option tend to be confusing for users. I see the motivation here, but it still applies. What's the rationale for not integrating this into TID252?
  • I'd have to double check if this rule doesn't conflict with any other rule (that's not really a concern, only a thing that needs doing)

@MichaReiser MichaReiser added rule Implementing or modifying a lint rule needs-decision Awaiting a decision from a maintainer labels Mar 16, 2025
@noirbizarre
Copy link
Author

noirbizarre commented Mar 16, 2025

Hi 👋🏼

Thanks for the prompt response.

I'll answer the bullet points in the same order:

It creates a new rule that doesn't exist upstream. We've made exceptions for some categories in the past but it comes with its own set of problems, which is why we're more hesitant to keep doing that (e.g. what if upstream adds a TID254...)

I took the liberty of submitting this pull request as the TID namespace already diverged from the upstream (TID253 doesn't exist in flake8-tidy-import), so this PR doesn't introduce this problem, it takes benefit from the fact the problem already exists (I know, weird thinking).
I actually submitted an upstream PR 2 years ago (adamchainz/flake8-tidy-imports#441), I didn't have feedback since, and I stop hoping for (I didn't insist either as the maintainer doesn't seem happy about it, and I didn't want to pressure him)

Rules that require explicit opt-in with an option tend to be confusing for users. I see the motivation here, but it still applies. What's the rationale for not integrating this into TID252?

Actually, I can, the initial PR (#3986) was doing that.
It was more about the semantic: the rule is advertising "Prefer absolute imports over relative imports from parent modules" and this PR is doing the opposite for siblings. It would mean that the same rule could say "Prefer absolute imports over relative imports from parents modules" and "Prefer relative imports over absolute imports for siblings modules".
But if this is acceptable, I'll rework this PR to make this a TID252 setting and change the documentation sentence for something more generic.

I'd have to double check if this rule doesn't conflict with any other rule (that's not really a concern, only a thing that needs doing)

If TID252 ban-relative-imports is set to all, it is by design contradictory with this rule. That's why I documented that it should be set to parents in the docstring.

What would be acceptable to you if I just integrate it with TID252?

  1. Simply use a 3rd ban-relative-import value like parents-with-relative-siblings with would ensure no conflict at the cost of being a bit contradictory with the ban term
  2. Keep the separate setting with a warning about the conflict?
  3. Another better approach

I'll do any necessary change to have this feature merged because I still have the need for this and from #1014, I don't seem to be the only one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-decision Awaiting a decision from a maintainer rule Implementing or modifying a lint rule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Relative import management
2 participants








ApplySandwichStrip

pFad - (p)hone/(F)rame/(a)nonymizer/(d)eclutterfier!      Saves Data!


--- a PPN by Garber Painting Akron. With Image Size Reduction included!

Fetched URL: http://github.com/astral-sh/ruff/pull/16772

Alternative Proxies:

Alternative Proxy

pFad Proxy

pFad v3 Proxy

pFad v4 Proxy