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

feat: module name shadowing #121

Merged
merged 7 commits into from
Mar 29, 2024

Conversation

asfaltboy
Copy link
Contributor

@asfaltboy asfaltboy commented Feb 5, 2024

Introduces check A005, that returns an error if a scanned filename matches a module name in the list of known builtin/stdlib python modules. An example of an error:

./temp/logging.py:0:1: A005 the module is shadowing a Python builtin module "logging"

We use sys.stdlib_module_names from python 3.10 as it contains all known module names (for all platforms), and on older versions, we fall back to sys.builtin_module_names which contains module names for the used python executable's platform.

Todos

  • agree on fallback to builtin_module_names in < 3.10
  • test skipped module names
  • update documentation?
  • update release notes?

@asfaltboy asfaltboy force-pushed the ps/module-name-shadowing branch 2 times, most recently from 06a3280 to f2b1455 Compare February 20, 2024 16:50
@asfaltboy asfaltboy marked this pull request as ready for review February 20, 2024 16:51
@asfaltboy
Copy link
Contributor Author

Hi @gforcada, hope you are doing good 👋 I have tested this PR's code manually with flake8, and it seems to work well. So, it's ready for your review. I left a few To-dos for things we may need to decide on before we merge the change

Copy link
Owner

@gforcada gforcada left a comment

Choose a reason for hiding this comment

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

It all looks good, I started the GHA, let's see what's their opinion as well 🤞🏾 😄

I only have one small remark 😃

flake8_builtins.py Outdated Show resolved Hide resolved
@gforcada
Copy link
Owner

Regarding the todos:

  • I agree on the fallback, or I would even ignore them, and only report it starting from 3.10, just to not give false positives, or different reports depending on the interpreter this is run ⚖️
  • skipped module names: that would be great, to test that the command line option is used according to expectations 🍀
  • documentation: please yes, updating the README is enough 👍🏾
  • release notes: indeed, updating CHANGES would be great

Copy link
Owner

@gforcada gforcada left a comment

Choose a reason for hiding this comment

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

Great job! thanks! 🙇🏾 😄

@gforcada gforcada merged commit df6c1d2 into gforcada:main Mar 29, 2024
7 checks passed
toofar added a commit to qutebrowser/qutebrowser that referenced this pull request Apr 8, 2024
flake8 got a new warning about a module name shadowing a builtin module: gforcada/flake8-builtins#121

Probably would have been safe enough to ignore it. But I don't think
moving it is that hard anyway. Hopefully I didn't miss anything!
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