pep8-naming icon indicating copy to clipboard operation
pep8-naming copied to clipboard

"camelcase imported as lowercase" failure in an import of a package or module

Open ericbn opened this issue 2 years ago • 12 comments

For example, this code:

import SalesforcePy.commons as sfc

fails with:

./test.py:1:2: N813 camelcase 'SalesforcePy.commons' imported as lowercase 'sfc'

Shouldn't pep-naming just ~look at the package name being imported (commons) instead of looking at the parent package names too~ consider this is an import of a package or module, and then not fail in this case?

I'm assuming this rule exists for when a class name is being imported, as discussed in https://stackoverflow.com/a/37590149/2654518. In this case:

from SalesforcePy.commons import BaseRequest as baserequest

sure should fail with:

./test.py:1:2: N813 camelcase 'BaseRequest' imported as lowercase 'baserequest'

ericbn avatar May 27 '22 17:05 ericbn

Here are some test cases I believe exemplify the possible scenarios, based on the existing test cases:

#: Okay
import mod.good as nice
#: Okay
import Mod.good as nice
#: Okay
import mod.NICE as GOOD
#: Okay
import mod.Camel as Memel
#: N811:1:1
import mod.GOOD as bad
#: N812:1:1
import mod.good as Bad
#: N812:1:1
import Mod.good as Bad
#: N813:1:1
import mod.CamelCase as noncamel
#: N814:1:1
import mod.CamelCase as CONSTANT
#: N817:1:1
import mod.CamelCase as CC

ericbn avatar May 31 '22 22:05 ericbn

Fix should be as simple as

diff --git a/src/pep8ext_naming.py b/src/pep8ext_naming.py
index 2755aaa..b2ad238 100644
--- a/src/pep8ext_naming.py
+++ b/src/pep8ext_naming.py
@@ -396,7 +396,7 @@ class ImportAsCheck(BaseASTCheck):
             asname = name.asname
             if not asname:
                 continue
-            original_name = name.name
+            original_name = name.name.rsplit('.', 1)[-1]
             err_kwargs = {'name': original_name, 'asname': asname}
             if original_name.isupper():
                 if not asname.isupper():

Or

diff --git a/src/pep8ext_naming.py b/src/pep8ext_naming.py
index 2755aaa..b33eb78 100644
--- a/src/pep8ext_naming.py
+++ b/src/pep8ext_naming.py
@@ -397,6 +397,7 @@ class ImportAsCheck(BaseASTCheck):
             if not asname:
                 continue
             original_name = name.name
+            original_name = original_name[original_name.rfind('.') + 1:]
             err_kwargs = {'name': original_name, 'asname': asname}
             if original_name.isupper():
                 if not asname.isupper():

for something maybe faster and less clean.

I'm more than happy to create a PR once the idea is approved.

ericbn avatar May 31 '22 22:05 ericbn

Holding a sleeping newborn so I'm going to be terse and respond to exactly I've point you made

Shouldn't pep-naming just look at the package name being imported (commons) instead of looking at the parent package names too, and then not fail in this case?

In the example you gave you otherwise would have had a camelcase name to reference everywhere SalesforcePy.commons. You aliases it as a lowercase. I don't think that is a valid example of what you're trying to prove

sigmavirus24 avatar Jun 01 '22 00:06 sigmavirus24

Congratulations on your newborn! Don't mind paying attention to this issue at least for the next 3 years or so! :- )


Let me try to rephrase the issue. In from mod import NICE as GOOD and import mod.NICE as GOOD my understanding is that "mod.NICE is being bound as GOOD" in both cases. As least this is my interpretation from the import statement documentation, when it exemplifies:

import foo.bar.baz as fbb  # foo, foo.bar, and foo.bar.baz imported, foo.bar.baz bound as fbb
from foo.bar import baz    # foo, foo.bar, and foo.bar.baz imported, foo.bar.baz bound as baz

The issue is that from mod import NICE as GOOD does not yield any linting error, but import mod.NICE as GOOD does. Same for any other variations where "nice" and "good" have the same type of case. I'm thinking both constructs are equivalent, so linting should yield the same result for both (no error).

ericbn avatar Jun 01 '22 16:06 ericbn

So i think the disconnect between you and me is based on what an import might be used like if not allowed to a different casing.

Two examples you have have a fully qualified band that is now mixed case without any alias. Using the Salesforce library to start with, I believe that changing that to from SalesforcePy import common as sfc wouldn't trigger the error because without the alias you'd refer to that as common otherwise. Do you see my point?

I do think the import lower. GOOD as good is trickier given convention that no one would import a constant like that.

Again, she caveats if holding a newborn and replying on mobile with one have

sigmavirus24 avatar Jun 01 '22 18:06 sigmavirus24

I see what you mean. from mod import NICE allows referring to NICE directly, while import mod.NICE makes you refer to mod.NICE.

from SalesforcePy import common as sfc works, but I prefer import SalesforcePy.common as sfc because SalesforcePy.common are modules. I'm just aliasing those, like suggested here to import very.deep.module as mod.

Maybe we can simplify the question to: When you look at mod.NICE do you see an upper case name coming from a lower case name, or a camel case name? Likewise, what about import mod.NICE? PEP8 does not say anything about this specifically, and at the end this is just about dealing with non-PEP8-standard module names, like SalesforcePy in the wild...

ericbn avatar Jun 01 '22 21:06 ericbn

I think I have a solution that makes more sense, in PEP8 terms, than the discussion I started above (and sorry if that was confusing).

Given that

when using from package import item, the item can be either a submodule (or subpackage) of the package, or some other name defined in the package, like a function, class or variable.

and

when using syntax like import item.subitem.subsubitem, each item except for the last must be a package; the last item can be a module or a package but can’t be a class or function or variable defined in the previous item.

(from https://docs.python.org/3/tutorial/modules.html#packages)

Then I propose:

  • When using from package import item as name, enforce that item and name have the same case type. This can be kept in rules N81{1,2,3,4,7}. They will just need to be more specific.
  • When using import item.subitem.subsubitem as name or import item as name, enforce that name is lowercase, because the last item can only be a module or a package. This will be a new rule (N819?).

ericbn avatar Jun 02 '22 15:06 ericbn

Then I propose:

  • When using from package import item as name, enforce that item and name have the same case type. This can be kept in rules N81{1,2,3,4,7}. They will just need to be more specific.

  • When using import item.subitem.subsubitem as name or import item as name, enforce that name is lowercase, because the last item can only be a module or a package. This will be a new rule (N819?).

I think those generally sound like good rules, they sort of fall outside the bounds of PEP 8. The Package and Module Names section tells us how a (source) module should be named, but I don't know if we should be adding enforcement to import'ing side.

Your second proposed rule feels closest in spirit to PEP 8. I'm less sure about the first one.

jparise avatar Jun 07 '22 02:06 jparise

The Package and Module Names section tells us how a (source) module should be named, but I don't know if we should be adding enforcement to import'ing side.

Agree. That's the point of the second proposed rule (not to force the case type to the importing side, only on the alias name side, since there are non-lowercase package names out in the wild, e.g. SalesforcePy)

If we also want to enforce the packages and modules in the code being linted to be lowercase, maybe we could check the directory and file names. But that might be a separate discussion...

Your second proposed rule feels closest in spirit to PEP 8. I'm less sure about the first one.

What are you not sure about the first proposed rule? The way I see it, rules N81{1,2,3,4,7} are already enforcing that aliases have the same case type as the imported item. I'm proposing these rules to be more specific (only be applied in imports in the form from package import item as name), and then to have a separate rule to specifically cover the module/package aliasing (the second proposed rule, to only be applied in imports in the form import item.subitem.subsubitem as name or import item as name).

ericbn avatar Jun 07 '22 15:06 ericbn

Your second proposed rule feels closest in spirit to PEP 8. I'm less sure about the first one.

What are you not sure about the first proposed rule? The way I see it, rules N81{1,2,3,4,7} are already enforcing that aliases have the same case type as the imported item. I'm proposing these rules to be more specific (only be applied in imports in the form from package import item as name), and then to have a separate rule to specifically cover the module/package aliasing (the second proposed rule, to only be applied in imports in the form import item.subitem.subsubitem as name or import item as name).

I understand better now. I thought you were proposing that we change the check to enforce that the alias simply matches the case of the imported name, skipping the other naming rules.

jparise avatar Jun 07 '22 22:06 jparise

Cool. Let me work on the code changes and propose then in a PR.

ericbn avatar Jun 08 '22 02:06 ericbn

Hi. Can we have a second look at this? Maybe I didn't express myself well before.

The goal is to check the PEP 8 package and module names convention is applied to aliases of imported packages or modules.

So given import item as alias, the rule will make sure that alias is always lowercase -- regardless if the case of item --, considering alias is a package or module (alias) name and should follow:

Modules should have short, all-lowercase names. Underscores can be used in the module name if it improves readability. Python packages should also have short, all-lowercase names, although the use of underscores is discouraged.

We won't check for the case of item because developers won't have control of that name in case of third-party packages, but we will check the case of alias because that was a name given by the developer of the current code.

EDIT: This rule will be specific for the import item as alias syntax. Given that:

when using syntax like import item.subitem.subsubitem, each item except for the last must be a package; the last item can be a module or a package but can’t be a class or function or variable defined in the previous item.

(from https://docs.python.org/3/tutorial/modules.html#packages)

ericbn avatar Feb 28 '23 14:02 ericbn