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

Add N810 specifically for package or module imports #203

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ericbn
Copy link

@ericbn ericbn commented Jun 9, 2022

These are differences in behavior based on existing and new test cases:

statement before now
import os as OS N812 lowercase 'os' imported as non lowercase 'OS' N810 package or module 'os' imported as non lowercase 'OS'
import GOOD as good N811 constant 'GOOD' imported as non constant 'good' Okay (because good is an alias of a package or module, not an alias of a constant)
import good as BAD N812 lowercase 'good' imported as non lowercase 'BAD' N810 package or module 'good' imported as non lowercase 'BAD'
import good as Bad N812 lowercase 'good' imported as non lowercase 'Bad' N810 package or module 'good' imported as non lowercase 'Bad'
import GOOD as Γ Okay N810 package or module 'GOOD' imported as non lowercase 'Γ'

Hope it makes sense based on the discussion in #201.

@ericbn
Copy link
Author

ericbn commented Jun 10, 2022

EDIT: Updated the rule code to N810, as it brings it closer to the other related rules.

as it brings the number closer to the other related rules.
@ericbn ericbn changed the title Add N819 specifically for package or module imports Add N810 specifically for package or module imports Jun 10, 2022
Copy link
Member

@jparise jparise left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing that I don't like about this is the way that (some? most?) existing N811/N812 errors will become a new N810 error. That'll be a tough backwards-incompatible upgrade for users who have specifically opted in/out of those error codes, or who have specifically suppressed them using # noqa comments.

src/pep8ext_naming.py Outdated Show resolved Hide resolved
@ericbn
Copy link
Author

ericbn commented Mar 6, 2023

Users will only get the N810 error if they're importing a package or module with a non-lowercase alias name with the import ... as ... syntax specifically, which can only be used to import packages or modules. This is arguably not the most common error users will have in their import statements. If they upgrade pep8-naming to a version with the new N810 rule they'll start getting a message that they were violating PEP 8 in those situations, which I hope will mean an improvement.

This also fixes an error when trying to correctly alias a non-lowercase package or module name with a lowercase alias name following PEP 8. I've encountered SalesforcePy in my daily work and their documentation recommends import SalesforcePy as sfdc. This currently fails with N813 camelcase 'SalesforcePy' imported as lowercase 'sfdc' and won't fail anymore with the changes being proposed here (because sfdc is clearly an alias of a package, not an alias of a class name).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants