ICN001 import-alias-is-not-conventional should check "from" imports #2070

Zeddicus414 · 2023-01-21T19:43:26Z

See #2047

This might be a naive implementation, since I copied and pasted the code block from the `Stmt::Import` section.

Next step, actually add real tests.

charliermarsh · 2023-01-21T19:44:00Z

Thanks! I'll review this in a bit.

Zeddicus414 · 2023-01-21T19:48:06Z

Couple of things. I know my commit history is pretty messy. I'm assuming that's something you can just discard during the PR process?

Also, I modified resources/test/fixtures/pyproject.toml and the associated src/settings/pyproject.rs tests before realizing that the insta tests set their own config, so those changes were not needed. I can undo these changes if that's preferred.

charliermarsh · 2023-01-21T19:49:44Z

Yup, commit history is no problem at all. By default, I squash the history, so this ends up as one commit (linked to the PR).
Yeah, if you don't mind, I'd prefer to roll back those changes to pyproject.toml.

src/checkers/ast.rs

resources/test/fixtures/flake8_import_conventions/from_imports.py

charliermarsh · 2023-01-21T20:09:57Z

resources/test/fixtures/flake8_import_conventions/from_imports.py

+
+import xml.dom.minidom as md
+from xml.dom import minidom as md
+from xml.dom.minidom import parseString as pstr


Maybe add a test for an import from with multiple members, only one of which needs to be flagged?

Adding test cases was a good call.

Currently, it considers this a violation:

import xml.dom.minidom

But it doesn't catch these:

from xml.dom import minidom from xml.dom.minidom import parseString

I'll try to track down what's up. I think these examples should all be violations.

Keeps the string from being created unless it's needed.

Zeddicus414 · 2023-01-21T20:50:22Z

Oops, looked like you merged the PR while I was still trying to add some more test conditions. Would you like me to just create a separate PR for that? Or work on it here?

charliermarsh · 2023-01-21T20:51:42Z

Gah sorry, yeah, I jumped the gun. Let’s do it in a separate PR please.

Zeddicus414 · 2023-01-21T20:53:48Z

No big deal. I think everything still works fine in main, there is just the slight inconsistency between the import and from...import cases where no alias is defined.

#2070 (comment)

charliermarsh · 2023-01-21T21:32:18Z

src/checkers/ast.rs

@@ -1231,6 +1231,28 @@ where
                    }

                    if let Some(asname) = &alias.node.asname {


In case you haven't caught it: this block only runs when the as is set (e.g. from foo import bar as baz, but not from foo import bar). I think you can lift all these changes out of this if, maybe?

ohhhhhhh, yeah, I see it now. I put it under that condition because I thought the original call was under the same clause on line 896, but I miss-read it. It's not under the if let Some(asname) = &alias.node.asname so mine shouldn't be either. Makes sense.

(Just for clarity in communication: do you plan to fix this, or want me to?)

[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [ruff](https://togithub.com/charliermarsh/ruff) | `^0.0.229` -> `^0.0.230` | [![age](https://badges.renovateapi.com/packages/pypi/ruff/0.0.230/age-slim)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://badges.renovateapi.com/packages/pypi/ruff/0.0.230/adoption-slim)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://badges.renovateapi.com/packages/pypi/ruff/0.0.230/compatibility-slim/0.0.229)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://badges.renovateapi.com/packages/pypi/ruff/0.0.230/confidence-slim/0.0.229)](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>charliermarsh/ruff</summary> ### [`v0.0.230`](https://togithub.com/charliermarsh/ruff/releases/tag/v0.0.230) [Compare Source](https://togithub.com/charliermarsh/ruff/compare/v0.0.229...v0.0.230) #### What's Changed - fix: pin rustpython to the same revision to fix cargo vendor by [@figsoda](https://togithub.com/figsoda) in [astral-sh/ruff#2069 - feat: implementation for TRY004 by [@sbrugman](https://togithub.com/sbrugman) in [astral-sh/ruff#2066 - ICN001 import-alias-is-not-conventional should check "from" imports by [@Zeddicus414](https://togithub.com/Zeddicus414) in [astral-sh/ruff#2070 - Update link to Pylint parity tracking issue by [@cosmojg](https://togithub.com/cosmojg) in [astral-sh/ruff#2074 - ICN001 check from imports that have no alias by [@Zeddicus414](https://togithub.com/Zeddicus414) in [astral-sh/ruff#2072 - Index source code upfront to power (row, column) lookups by [@charliermarsh](https://togithub.com/charliermarsh) in [astral-sh/ruff#1990 - Remove remaining `ropey` usages by [@charliermarsh](https://togithub.com/charliermarsh) in [astral-sh/ruff#2076 - Include package path in cache key by [@charliermarsh](https://togithub.com/charliermarsh) in [astral-sh/ruff#2077 - feat: update scripts to new rules structure by [@sbrugman](https://togithub.com/sbrugman) in [astral-sh/ruff#2078 - Base `INP` check on package inference by [@charliermarsh](https://togithub.com/charliermarsh) in [astral-sh/ruff#2079 - Improve generator precedence operations by [@charliermarsh](https://togithub.com/charliermarsh) in [astral-sh/ruff#2080 - Support decorators in source code generator by [@charliermarsh](https://togithub.com/charliermarsh) in [astral-sh/ruff#2081 - feat: enable autofix for TRY004 by [@sbrugman](https://togithub.com/sbrugman) in [astral-sh/ruff#2084 - Refactor, decouple and support "PL" by [@not-my-profile](https://togithub.com/not-my-profile) in [astral-sh/ruff#2051 - \[`pep8-naming`]\[`N806`] Don't mark `TypeVar` & `NewType` Assignment as Errors by [@saadmk11](https://togithub.com/saadmk11) in [astral-sh/ruff#2085 - Update linters pypi links to latest version by [@alonme](https://togithub.com/alonme) in [astral-sh/ruff#2062 - flake8\_to_ruff: support `isort` options by [@shannonrothe](https://togithub.com/shannonrothe) in [astral-sh/ruff#2082 - Update RustPython to fix `Dict.keys` type by [@harupy](https://togithub.com/harupy) in [astral-sh/ruff#2086 #### New Contributors - [@figsoda](https://togithub.com/figsoda) made their first contribution in [astral-sh/ruff#2069 - [@cosmojg](https://togithub.com/cosmojg) made their first contribution in [astral-sh/ruff#2074 - [@alonme](https://togithub.com/alonme) made their first contribution in [astral-sh/ruff#2062 - [@shannonrothe](https://togithub.com/shannonrothe) made their first contribution in [astral-sh/ruff#2082 **Full Changelog**: astral-sh/ruff@v0.0.229...v0.0.230 </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. --- - [ ] 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).  Signed-off-by: Renovate Bot <bot@renovateapp.com> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

Zeddicus414 added 6 commits January 20, 2023 21:47

Add ICN001 import convention check to from imports

5b9a484

This might be a naive implementation, since I copied and pasted the code block from the `Stmt::Import` section.

Use the full name for the lint.

04cccba

Add some test data for ICN001.

e42661b

Next step, actually add real tests.

Update the pyproject.toml parse test to include the new options.

b0682a9

Rename from_imports. Add some good cases.

6e81144

Add insta tests for ICN001 checking "from" style imports.

cdeb882

Undo changes to pyproject.toml and associated tests

4e14de2

charliermarsh reviewed Jan 21, 2023

View reviewed changes

src/checkers/ast.rs Outdated Show resolved Hide resolved

charliermarsh reviewed Jan 21, 2023

View reviewed changes

resources/test/fixtures/flake8_import_conventions/from_imports.py Show resolved Hide resolved

charliermarsh reviewed Jan 21, 2023

View reviewed changes

Zeddicus414 added 2 commits January 21, 2023 14:21

Move the full_name into the ImportAliasIsNotConventional block.

e47c7ea

Keeps the string from being created unless it's needed.

Fix formatting.

dc10ed3

charliermarsh merged commit 5726118 into astral-sh:main Jan 21, 2023

charliermarsh reviewed Jan 21, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ICN001 import-alias-is-not-conventional should check "from" imports #2070

ICN001 import-alias-is-not-conventional should check "from" imports #2070

Zeddicus414 commented Jan 21, 2023

charliermarsh commented Jan 21, 2023

Zeddicus414 commented Jan 21, 2023

charliermarsh commented Jan 21, 2023

charliermarsh Jan 21, 2023

Zeddicus414 Jan 21, 2023

Zeddicus414 commented Jan 21, 2023

charliermarsh commented Jan 21, 2023

Zeddicus414 commented Jan 21, 2023

charliermarsh Jan 21, 2023

Zeddicus414 Jan 21, 2023

charliermarsh Jan 21, 2023

		@@ -1231,6 +1231,28 @@ where
		}

		if let Some(asname) = &alias.node.asname {

ICN001 import-alias-is-not-conventional should check "from" imports #2070

ICN001 import-alias-is-not-conventional should check "from" imports #2070

Conversation

Zeddicus414 commented Jan 21, 2023

charliermarsh commented Jan 21, 2023

Zeddicus414 commented Jan 21, 2023

charliermarsh commented Jan 21, 2023

charliermarsh Jan 21, 2023

Choose a reason for hiding this comment

Zeddicus414 Jan 21, 2023

Choose a reason for hiding this comment

Zeddicus414 commented Jan 21, 2023

charliermarsh commented Jan 21, 2023

Zeddicus414 commented Jan 21, 2023

charliermarsh Jan 21, 2023

Choose a reason for hiding this comment

Zeddicus414 Jan 21, 2023

Choose a reason for hiding this comment

charliermarsh Jan 21, 2023

Choose a reason for hiding this comment