Implement TID251 (banning modules & module members) #1436
Conversation
| pub struct Settings { | ||
| pub ban_relative_imports: Strictness, | ||
| pub banned_api: FxHashMap<String, BannedApi>, |
There was a problem hiding this comment.
I know you explicitly preferred banned_api, but given the minor difference, it's worth it to me to maintain compatibility with flake8-tidy-imports and keep this as banned-modules. How do you feel about that?
There was a problem hiding this comment.
Yes I don't like banned-modules because it's misleading since the setting can also be used for module members. I do think correct semantics make a big difference in terms of usability. And I don't get your point about compatibility given that you cannot copy'n'paste flake8-tidy-imports config into the ruff config anyway because:
- flake8-tidy-imports does not use TOML but .cfg which uses a different syntax.
- You have to wrap the names in double quotes and appent
.msg.
If we still want both settings to have the same name despite having different formats, I think it would make more sense if flake8-tidy-imports updated its setting name to banned-api since it's the more correct terminology.
There was a problem hiding this comment.
I guess this is fine. There is some mental overhead to deviating (folks that migrate their config have to know to change the setting name) but I understand your point, and I don't feel strongly.
What's your reaction to banned import? There was some discussion here and it looks like that's the verbiage that flake8-tidy-imports uses in their messages.
There was a problem hiding this comment.
I was thinking about this a bit more, and I prefer banned-imports over banned-api. The issue with banned-api is that it's too general, and actually implies things that go beyond what the tool can do. For example, to me, banned-api would also include things like method calls.
There was a problem hiding this comment.
(If you agree, I'm also happy to do the work of renaming, then merge the PR.)
There was a problem hiding this comment.
Yes "banned import" would be accurate for flake8-tidy-imports but only because they don't yet implement the attribute access check (adamchainz/flake8-tidy-imports/issues/63), which this PR implements. Since the check also checks for attribute access "banned import" isn't accurate, for example:
import foo
foo.bar # TIDI201 Banned import 'foo.bar' usedfoo.bar here is not an import.
|
This looks great! Some minor comments. Thank you for the PR! |
[](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [ruff](https://togithub.com/charliermarsh/ruff) | `^0.0.200` -> `^0.0.201` | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>charliermarsh/ruff</summary> ### [`v0.0.201`](https://togithub.com/charliermarsh/ruff/releases/tag/v0.0.201) [Compare Source](https://togithub.com/charliermarsh/ruff/compare/v0.0.200...v0.0.201) #### What's Changed - Rename config to settings in the playground by [@​charliermarsh](https://togithub.com/charliermarsh) in [astral-sh/ruff#1450 - docs(README): add missing `flake8-simplify` by [@​mkniewallner](https://togithub.com/mkniewallner) in [astral-sh/ruff#1449 - Add Sphinx to user list by [@​AA-Turner](https://togithub.com/AA-Turner) in [astral-sh/ruff#1451 - Move default options into WASM interface by [@​charliermarsh](https://togithub.com/charliermarsh) in [astral-sh/ruff#1453 - Implement dark mode by [@​charliermarsh](https://togithub.com/charliermarsh) in [astral-sh/ruff#1455 - Use trailingComma: 'all' by [@​charliermarsh](https://togithub.com/charliermarsh) in [astral-sh/ruff#1457 - Remove generated TypeScript options by [@​charliermarsh](https://togithub.com/charliermarsh) in [astral-sh/ruff#1456 - Copy URL but don't update the hash by [@​charliermarsh](https://togithub.com/charliermarsh) in [astral-sh/ruff#1458 - Removed unicode literals by [@​colin99d](https://togithub.com/colin99d) in [astral-sh/ruff#1448 - Implement TID251 (banning modules & module members) by [@​not-my-profile](https://togithub.com/not-my-profile) in [astral-sh/ruff#1436 - Implicit flake8-implicit-str-concat by [@​charliermarsh](https://togithub.com/charliermarsh) in [astral-sh/ruff#1463 #### New Contributors - [@​mkniewallner](https://togithub.com/mkniewallner) made their first contribution in [astral-sh/ruff#1449 - [@​AA-Turner](https://togithub.com/AA-Turner) made their first contribution in [astral-sh/ruff#1451 - [@​not-my-profile](https://togithub.com/not-my-profile) made their first contribution in [astral-sh/ruff#1436 **Full Changelog**: astral-sh/ruff@v0.0.200...v0.0.201 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR is behind base branch, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://app.renovatebot.com/dashboard#github/ixm-one/pytest-cmake-presets). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC43NC4yIiwidXBkYXRlZEluVmVyIjoiMzQuNzQuMiJ9--> Signed-off-by: Renovate Bot <bot@renovateapp.com> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
](https://renovatebot.com){kind=link}
Resolves #1422.