ICN001 and N817 don't play with each other

[ember91]

List of keywords searched

• ICN001
• N817

Minimal code snippets

test1.py

import xml.etree.ElementTree as Et

test2.py

import xml.etree.ElementTree as ET

Invoked commands

ruff check test1.py test2.py

Current ruff settings

select = ["ICN001", "N817"]

Current ruff version

ruff 0.6.0

Description

It seems like N817 wants me to import it as Et while ICN001 wants me to import it as ET. Perhaps it is ok that rules conflict in ruff. If so I guess this is not a bug.

[MichaReiser]

Thanks. No, that's a bug. Rules should be compatible out of the box.

I see two options here:

1. N817 to ignore any names configured in checker.settings.flake8_import_conventions.aliases when ICN001 is enabled
2. ICN001 to ignore all camel case names configured in checker.settings.flake8_import_conventions.aliases if N817 is enabled.

I prefer 1. because it involves less conditional logic (it ignores all the names rather than some).

@charliermarsh what do you think?

[cclauss]

I also prefer 1.

% echo "from xml.etree import ElementTree" | ruff check --select=ALL --ignore=D100,F401 -

-:1:23: ICN001 `xml.etree.ElementTree` should be imported as `ET`
  |
1 | from xml.etree import ElementTree
  |                       ^^^^^^^^^^^ ICN001

% echo "import xml.etree.ElementTree as ET" | ruff check --select=ALL --ignore=D100,F401 -

-:1:8: N817 CamelCase `ElementTree` imported as acronym `ET`
  |
1 | import xml.etree.ElementTree as ET
  |        ^^^^^^^^^^^^^^^^^^^^^^^^^^^ N817

[NMertsch]

I also prefer 1. Not because of the implementation logic, but because it favors the specific rule ICN001 over the generic rule N817:

• N817: Generally, don't use acronyms for camel-case imports, like MCN for MyClassName.
• ICN001: For ElementTree specifically, use the alias ET because that's the convention for this module.

[AlexWaygood]

(1) sounds good to me too

[ember91]

Wow! You are so responsive in this repo. Hats off!

[hofbi]

@MichaReiser I still see N817 CamelCase ElementTree imported as acronym ET on v0.6.1. If I understand #12922 correctly, it should now be import ElementTree as XET? However, the autofix of ICN001 still autofixes to import ElementTree as ET. Do I miss anything?

[cclauss]

I have never seen XET in the wild and the docs/tutorial recommends ET.

https://docs.python.org/3/library/xml.etree.elementtree.html#tutorial

[AlexWaygood]

@hofbi / @cclauss: We are definitely not recommending XET as an alias for xml.etree.ElementTree in this rule! 😄

The tests added in #12922 are specifically testing that the "conventional aliases" setting can be used to configure this rule. One fixture is being run with the "conventional aliases" setting being set to "xml.etree.ElementTree": "XET", and the snapshot you see as a result shows you what would happen if you ran Ruff with that (strange) configuration

[hofbi]

Ok makes sense for XET. Then do I have to configure anything so that both check play nicely together. Right now I still see the N817 error after the autofix adding ET.

[MichaReiser]

Ok makes sense for XET. Then do I have to configure anything so that both check play nicely together. Right now I still see the N817 error after the autofix adding ET.

There's a separate fix for from ... import that has not been released yet #12946

If the incompatibility that you're seeing isn't related to from ... import, please open a new issue and share a code example with us. Happy to take a look.

[hofbi]

Oh I missed that one. My issue is definitely #12940. So just waiting for the next release to get this fixed, thanks.

[MichaReiser]

No worries. I missed that one too when implanting the fix 🥲