-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Add no-dict-direct-access checker #6901
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
Conversation
for more information, see https://pre-commit.ci
The new checker seems to be enabled in the tests by default (which makes them fail). I'm not sure what the best course of action is here, or if the checker is even in the right place. Can anyone assist? |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your interest in contributing this to pylint. I think at the least this should be an optional extension. See the examples in pylint/checkers/extensions
. So that will solve the bulk of the test failures.
You might wait to hear positive feedback from others before continuing, as this is the sort of thing that might be too strict even for an optional extension.
I don't have time for a full review right now but as Jacob said it needs to be an extension. We have really opinionated extensions already so it's ok. (While used, for example). |
Thanks @Pierre-Sassoulas for the vote of confidence and @jacobtylerwalls for the general hints! I'll move the check to an extension and am absolutely open to more opinions on the matter. Worst case, I'll release it on pypi or something and it'll be more unofficial. :) |
839da60
to
c713d57
Compare
093cb37
to
7d545e4
Compare
for more information, see https://pre-commit.ci
This comment has been minimized.
This comment has been minimized.
I have fixed a few issues and put the checker into an extension. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this need some functional tests, in particular what happen when the code is:
d = {}
try:
d["a"] = 1
except KeyError:
print("The 'a' key does not exists in d")
I added a functional test. I fails on my local machine, although I don't see where I went wrong. The code you showed doesn't make all that much sense, since One could try to check if, by going up the scope, one encounters an |
Right, my bad.
OK, I understand what you want to do now. it's very opinionated though, more so than the while-checker and even harmful performance wise. The 350+ violations in flask are indicative of that. I'll re-add the discussion label because I think we need to make "editorial choices" at some point to prevent users having to triage our extensions themselves. Pushed to the extreme this policy of adding very opinionated checkers that seems easy to maintain in pylint is not workable because it's creating noise in our doc and in the |
Thanks for taking the time to prepare a pull request. I agree, though, with Pierre that this is probably better as a plugin maintained outside the pylint project. If we hear more requests, I'll point folks here and revisit the issue. |
@jacobtylerwalls I understand, and have begun migrating this to a pylint plugin which will be released on pypi. Thanks for the pleasent review process :) |
Type of Changes
Description
I noticed that I rarely use direct dictionary access, as in
myfunction(foo["bar"])
, sincefoo
might not contain"bar"
. I usually go forfoo.get("bar")
(or some other default value) to be safe. I wanted a checker that checks my code for these direct dictionary subscripts and reports them. I found a few bugs that way already and thought this might fit into pylint in general.The following will produce a message:
Whereas this will not:
This is also fine:
Since assignment is safe per se.