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 allow-redefined-builtins option to variable checker #3643

Merged
merged 1 commit into from
Mar 5, 2021

Conversation

kapsh
Copy link
Contributor

@kapsh kapsh commented May 23, 2020

Steps

  • Add yourself to CONTRIBUTORS if you are a new contributor.
  • Add a ChangeLog entry describing what your PR does.
  • If it's a new feature or an important bug fix, add a What's New entry in doc/whatsnew/<current release.rst>.
  • Write a good description on what the PR does.

Description

Some builtins have little-to-no use in application code while being
convenient as variables names (e.g. id, dir). New option allows
to configure allowed to override names for redefined-builtin checker.

Type of Changes

Type
✨ New feature

Related Issue

Closes #3263

@kapsh
Copy link
Contributor Author

kapsh commented May 23, 2020

Very simple approach and only for local names. Not sure if module-level variables should be supported as well, though it makes sense just for consistency.

Guidance is appreciated.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 90.659% when pulling 5c581e5 on kapsh:allow-redefined-builtin into 1fc490c on PyCQA:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 90.659% when pulling 5c581e5 on kapsh:allow-redefined-builtin into 1fc490c on PyCQA:master.

@coveralls
Copy link

coveralls commented May 23, 2020

Coverage Status

Coverage increased (+0.001%) to 91.48% when pulling 942b02f on kapsh:allow-redefined-builtin into 25e63b0 on PyCQA:master.

Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

Thank you, the code is clean, and it has proper tests. This is mergeable. Why would you want to allow some redefined builtins though ?

tests/functional/r/redefined_builtin_allowed.rc Outdated Show resolved Hide resolved
@kapsh
Copy link
Contributor Author

kapsh commented May 27, 2020

@Pierre-Sassoulas the reason is explained in linked issue and commit message, I can add more details as I see them.
Builtin namespace in Python contains some objects useful in REPL but (probably) never called in real code. Notable examples are: id, dir, license. But when I am walking over files dir variable is often the best name, id is useful when working with ORMs, etc. This option would allow user to make per-project exceptions if they have a habit to use such names without totally disabling redefined-builtin check.

Thanks for inspecting the code. I'd also like to hear your opinion about including module level variables in ignore list. There are two more places in variables.py where this message is raised: visit_module and visit_globals (btw difference between them is unclear to me).
Pro: consistent pylint behavior, would look like a bug if option only works in functions.
Con: possible encouraging to write code with weirdly named globals.

@hippo91
Copy link
Contributor

hippo91 commented Sep 20, 2020

@Pierre-Sassoulas have you enough time to answer this?

@Pierre-Sassoulas
Copy link
Member

Sorry I missed your answer completely @kapsh. This make sense. Regarding your question, I'm not familiar with that part of the code, but looking at it from a functional perspective, I think a global named id or dir would make erroneous code easier to write because it would shadow the actual builtin. What do you think ?

@jhaux
Copy link

jhaux commented Feb 23, 2021

I would really love this feature. Is there a chance this might get merged soon?

@Pierre-Sassoulas
Copy link
Member

Pierre-Sassoulas commented Feb 23, 2021

I re-read the discussion, it seems to me that a global variable dir or id or a method is dangerous, but for a variable it's ok because the scope is limited and if you're going to use the builtin dir or id in a function there is a high probability that you will realize you have a variable with the same name shadowing it. So we could merge the MR after a rebase ? Thoughts @hippo91 @AWhetter ?

Copy link
Contributor

@AWhetter AWhetter left a comment

Choose a reason for hiding this comment

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

Whether or not we allow this functionality depends on how we view pylint as a whole. In the past pylint has been very rigid in what it will and won't accept. I've had feedback that this makes pylint seem "too opinionated" and some people don't like pylint as a result. So I have wanted pylint to move towards allowing things to be more configurable so that it encourages you to do the right thing by default but also give users the option to configure things more to their liking if they know what they are doing.
I think this pull request is a good step in that direction so I'm happy with it!

@Pierre-Sassoulas Pierre-Sassoulas added Needs review 🔍 Needs to be reviewed by one or multiple more persons Proposal 📨 Waiting on author Indicate that maintainers are waiting for a message of the author and removed Work in progress labels Feb 23, 2021
@jhaux
Copy link

jhaux commented Feb 24, 2021

That is great to hear! Thank you for reacting so fast!

@kapsh
Copy link
Contributor Author

kapsh commented Feb 25, 2021

@AWhetter I am relieved to hear that this feature is desirable. Let me just update it and resolve conflicts here.

@Pierre-Sassoulas agreed, shadowing in globals is easy to miss and shouldn't be permitted by this option. Tests will make it clearer.

@kapsh kapsh force-pushed the allow-redefined-builtin branch 2 times, most recently from 1732666 to 91ed3fe Compare February 25, 2021 10:13
@hippo91
Copy link
Contributor

hippo91 commented Mar 5, 2021

@kapsh thanks for this PR. I think it is definitely a good idea.

@hippo91
Copy link
Contributor

hippo91 commented Mar 5, 2021

@Pierre-Sassoulas do you approve?

@kapsh
Copy link
Contributor Author

kapsh commented Mar 5, 2021

Rebased again because of conflicts in changelog.

Some builtins have little-to-no use in application code while being
convenient as variables names (e.g. id, dir). New option allows
to configure allowed to override names for redefined-builtin checker.

Closes pylint-dev#3263
@Pierre-Sassoulas Pierre-Sassoulas merged commit 6830915 into pylint-dev:master Mar 5, 2021
@kapsh kapsh deleted the allow-redefined-builtin branch March 5, 2021 09:21
@Pierre-Sassoulas
Copy link
Member

Thank you for the merge request, definitely something that will make pylint less annoying to use !

@Pierre-Sassoulas Pierre-Sassoulas removed the Needs review 🔍 Needs to be reviewed by one or multiple more persons label Mar 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Proposal 📨 Waiting on author Indicate that maintainers are waiting for a message of the author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Suppress redefined-builtin for specific builtins
6 participants