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

pylint friendly #2417

Closed
DavidHwu opened this issue Jan 20, 2015 · 11 comments

Comments

Projects
None yet
5 participants
@DavidHwu
Copy link

commented Jan 20, 2015

Kindly request that requests be more pylint compliant. When evaluating which package to use, this comes up with a score of -154.

Global evaluation

Your code has been rated at -154.92/10 (previous run: -154.91/10, -0.00)

Errors:

Davids-MacBook-Pro-2:~ dhwu$ pylint -E /Users/dhwu/Desktop/Evals/HTTP\ Libs/requests-2.5.1/requests
No config file found, using default configuration
************* Module requests.compat
E: 88, 4: No name 'quote' in module 'urllib' (no-name-in-module)
E: 88, 4: No name 'unquote' in module 'urllib' (no-name-in-module)
E: 88, 4: No name 'quote_plus' in module 'urllib' (no-name-in-module)
E: 88, 4: No name 'unquote_plus' in module 'urllib' (no-name-in-module)
E: 88, 4: No name 'urlencode' in module 'urllib' (no-name-in-module)
E: 88, 4: No name 'getproxies' in module 'urllib' (no-name-in-module)
E: 88, 4: No name 'proxy_bypass' in module 'urllib' (no-name-in-module)
E: 98,10: Undefined variable 'unicode' (undefined-variable)
E: 99,17: Using variable 'basestring' before assignment (used-before-assignment)
E:100,26: Undefined variable 'long' (undefined-variable)
************* Module requests.models
E: 39,10: Instance of 'LookupDict' has no 'moved' member (no-member)
E: 40,10: Instance of 'LookupDict' has no 'found' member (no-member)
E: 41,10: Instance of 'LookupDict' has no 'other' member (no-member)
E: 42,10: Instance of 'LookupDict' has no 'temporary_redirect' member (no-member)
E: 43,10: Instance of 'LookupDict' has no 'permanent_redirect' member (no-member)
E:343,18: Undefined variable 'unicode' (undefined-variable)
E:634,74: Instance of 'LookupDict' has no 'moved_permanently' member (no-member)
E:634,99: Instance of 'LookupDict' has no 'permanent_redirect' member (no-member)
E:792,51: Instance of 'bool' has no 'decode' member (no-member)
************* Module requests.sessions
E:145,42: Instance of 'LookupDict' has no 'see_other' member (no-member)
E:151,41: Instance of 'LookupDict' has no 'found' member (no-member)
E:156,41: Instance of 'LookupDict' has no 'moved' member (no-member)
E:162,46: Instance of 'LookupDict' has no 'temporary_redirect' member (no-member)
E:162,72: Instance of 'LookupDict' has no 'permanent_redirect' member (no-member)
************* Module requests.utils
E:537,52: Module 'sys' has no 'pypy_version_info' member (no-member)
E:538,52: Module 'sys' has no 'pypy_version_info' member (no-member)
E:539,52: Module 'sys' has no 'pypy_version_info' member (no-member)
E:540,15: Module 'sys' has no 'pypy_version_info' member (no-member)
E:541,76: Module 'sys' has no 'pypy_version_info' member (no-member)
************* Module requests.packages.chardet
E: 23,52: Undefined variable 'unicode' (undefined-variable)
************* Module requests.packages.chardet.compat
E: 25,21: Undefined variable 'unicode' (undefined-variable)
************* Module requests.packages.urllib3.connection
E:159, 0: class already defined line 20 (function-redefined)
************* Module requests.packages.urllib3.connectionpool
E: 46,19: Instance of '_MovedItems' has no 'xrange' member (no-member)
************* Module requests.packages.urllib3.request
E: 4, 4: No name 'urlencode' in module 'urllib' (no-name-in-module)
E: 49, 8: NotImplemented raised - should raise NotImplementedError (notimplemented-raised)
************* Module requests.packages.urllib3.response
E:182,36: Instance of 'str' has no 'read' member (no-member)
E:186,36: Instance of 'str' has no 'read' member (no-member)
E:195,33: Instance of 'str' has no 'close' member (no-member)
E:296,21: Instance of 'str' has no 'close' member (no-member)
E:303,28: Instance of 'str' has no 'closed' member (no-member)
E:305,28: Instance of 'str' has no 'isclosed' member (no-member)
E:313,28: Instance of 'str' has no 'fileno' member (no-member)
E:320,28: Instance of 'str' has no 'flush' member (no-member)
************* Module requests.packages.urllib3.contrib.ntlmpool
E: 44,13: Instance of 'NTLMConnectionPool' has no 'num_connections' member (no-member)
E: 46,24: Instance of 'NTLMConnectionPool' has no 'num_connections' member (no-member)
E: 46,46: Instance of 'NTLMConnectionPool' has no 'host' member (no-member)
E: 53,41: Instance of 'NTLMConnectionPool' has no 'host' member (no-member)
E: 53,57: Instance of 'NTLMConnectionPool' has no 'port' member (no-member)
************* Module requests.packages.urllib3.contrib.pyopenssl
E: 58, 0: No name '_fileobject' in module 'socket' (no-name-in-module)
E:158,33: Instance of 'OctetString' has no 'getComponentByPosition' member (no-member)
E:158,33: Instance of 'Any' has no 'getComponentByPosition' member (no-member)
E:158,33: Instance of 'str' has no 'getComponentByPosition' member (no-member)
************* Module requests.packages.urllib3.packages.ordered_dict
E: 82,35: Instance of 'dict' has no 'itervalues' member (no-member)
E:142, 4: Method has no argument (no-method-argument)
E:165,29: Instance of 'tuple' has no 'keys' member (no-member)
************* Module requests.packages.urllib3.packages.six
E: 42,19: Undefined variable 'basestring' (undefined-variable)
E: 43,26: Undefined variable 'long' (undefined-variable)
E: 44,31: Module 'types' has no 'ClassType' member (no-member)
E: 45,16: Undefined variable 'unicode' (undefined-variable)
E: 84,22: Instance of '_LazyDescr' has no 'resolve' member (no-member)
E:294,15: Undefined variable 'unicode' (undefined-variable)
E:341,36: Undefined variable 'basestring' (undefined-variable)
E:347,31: Undefined variable 'unicode' (undefined-variable)
E:353,31: Undefined variable 'unicode' (undefined-variable)
E:361,35: Undefined variable 'unicode' (undefined-variable)
E:365,22: Undefined variable 'unicode' (undefined-variable)
E:366,20: Undefined variable 'unicode' (undefined-variable)
************* Module requests.packages.urllib3.util.connection
E: 87, 8: Raising NoneType while only classes or instances are allowed (raising-bad-type)
************* Module requests.packages.urllib3.util.ssl

E: 22, 4: No name 'OP_NO_SSLv2' in module 'ssl' (no-name-in-module)
E: 22, 4: No name 'OP_NO_SSLv3' in module 'ssl' (no-name-in-module)
E: 22, 4: No name 'OP_NO_COMPRESSION' in module 'ssl' (no-name-in-module)
E: 41, 4: class already defined line 7 (function-redefined)
Davids-MacBook-Pro-2:~ dhwu$

@Lukasa

This comment has been minimized.

Copy link
Collaborator

commented Jan 20, 2015

Opening this kind of issue is exactly the kind of thing that is likely to get you some very negative comments indeed. =) Offering criticism without offering to help improve it is extremely bad manners. It's worse when the criticism is of the form "Your project doesn't live up to my personal standard " as this is. It's worse still when your criticism is based on an automated tool that is wrong.

Allow me to pick some examples:

E: 88, 4: No name 'quote' in module 'urllib' (no-name-in-module)

That's only true in Python 3, and the whole purpose of this module is to cover over differences between Python 2 and 3. All these errors are wrong.

E: 39,10: Instance of 'LookupDict' has no 'moved' member (no-member)

Static analysis of Python is stupid: it's a dynamic language! In practice, the instance gains this member at import time: any tool smart enough to import the module would have known this. These errors are all wrong.

E:537,52: Module 'sys' has no 'pypy_version_info' member (no-member)

It does on PyPy. Given that this is guarded by an 'if pypy' check, this error and all the ones like it are wrong.

E: 23,52: Undefined variable 'unicode' (undefined-variable)

There is on Python 2.

Any of the errors on urllib3 are in the wrong module, which is even more obviously true of six.

I'm being as polite as I can when I say that your tool is doing a terrible job. I strongly advise you never to open a similar issue on a different repository, or you will likely encounter a much stronger negative reaction than this one.

@Lukasa Lukasa closed this Jan 20, 2015

@sigmavirus24

This comment has been minimized.

Copy link
Contributor

commented Jan 20, 2015

It looks like most of these are based around py2/3 compatibility which we have no intention of breaking.

@DavidHwu

This comment has been minimized.

Copy link
Author

commented Jan 20, 2015

@Lukasa : I am following a well known practice that is detailed in Idiomatic Python author:
http://www.jeffknupp.com/blog/2013/11/15/supercharge-your-python-developers/
Also detailed in Google coding standards.
FWIW: pylint has caught many errors in my code and others in the past.
While we may differ in our views, not placing value judgements. Just a data point.
Like I said in my prior post, my intent is to weight differing approaches. Request at the top of that list.

Was hoping that this kind ask would be more open to possible changes... looks like closed for discussion already.

If you guys are open, I'd be open to making changes once I had more time.
But then that would be un-welcomed based on your statement too.
my 2 cents

@Lukasa

This comment has been minimized.

Copy link
Collaborator

commented Jan 20, 2015

To be clear: I do not believe anything that pylint has printed for modules outside requests.packages (which are third party code) is valid. I think everything it has said is wrong. If you can point to something that is not wrong, I will happily consider accepting a change based on it.

I agree that pylint can catch errors. However, doing a drive-by dump of every line reported by pylint is not a helpful approach. Two notes for the future:

  1. Authors often have different views on Python style, particularly PEP8. Requests does not follow all of PEP8, and this is quite deliberate. You should always consult a project's author before raising a bug report about their code failing a linter.
  2. You should validate the concerns of the linter before raising them. Open source maintainers are often very busy people, and asking them to validate each line of the linter's output for false positives is a rude way to behave. If you'd looked at these 'errors' first you'd have seen that they aren't valid.
  3. It's likely that you'll say we should add a .pylintrc file to address these concerns. That's not a good idea. We should not have to add a dotfile to the repository root for every person who writes a linter.
@DavidHwu

This comment has been minimized.

Copy link
Author

commented Jan 20, 2015

Points taken

My apology for this faux pas

Sent from my iPhone

On Jan 20, 2015, at 10:19 AM, Cory Benfield notifications@github.com wrote:

To be clear: I do not believe anything that pylint has printed for modules outside requests.packages (which are third party code) is valid. I think everything it has said is wrong. If you can point to something that is not wrong, I will happily consider accepting a change based on it.

I agree that pylint can catch errors. However, doing a drive-by dump of every line reported by pylint is not a helpful approach. Two notes for the future:

Authors often have different views on Python style, particularly PEP8. Requests does not follow all of PEP8, and this is quite deliberate. You should always consult a project's author before raising a bug report about their code failing a linter.
You should validate the concerns of the linter before raising them. Open source maintainers are often very busy people, and asking them to validate each line of the linter's output for false positives is a rude way to behave. If you'd looked at these 'errors' first you'd have seen that they aren't valid.
It's likely that you'll say we should add a .pylintrc file to address these concerns. That's not a good idea. We should not have to add a dotfile to the repository root for every person who writes a linter.

Reply to this email directly or view it on GitHub.

@piotr-dobrogost

This comment has been minimized.

Copy link
Contributor

commented Jan 23, 2015

@Lukasa
Are you open to annotating source with comments disabling pylint warnings? Given that one can use symbolic names of warnings/errors such annotation would serve as documentation at the same time which should be useful.

@Lukasa

This comment has been minimized.

Copy link
Collaborator

commented Jan 23, 2015

@piotr-dobrogost Nope. =)

My concern is the potentially unbounded number of linters that we may be asked to support annotations or dotfiles for. Just because someone created a tool that they love doesn't mean we should add metadata to our codebase to support it. If the requests project uses a tool for its development, we'll add that metadata. Otherwise, we won't.

@sigmavirus24

This comment has been minimized.

Copy link
Contributor

commented Jan 23, 2015

@piotr-dobrogost Are you open to determining how many of those are false positives and constitute absolutely useless feedback from a linter? The majority of the errors are just plain wrong and any changes belong in pylint.

@adamtheturtle

This comment has been minimized.

Copy link

commented Dec 23, 2017

PyCQA/pylint#1411 is the pylint issue which unfortunately indicates that pylint will not support requests.codes.

However, a workaround has been suggested for users: generated-members, documented in https://pylint.readthedocs.io/en/latest/technical_reference/features.html#id34.

@Lukasa

This comment has been minimized.

Copy link
Collaborator

commented Dec 23, 2017

To be clear, Requests should not add generated-members support itself. If folks want to add it to their own CI that’s fine, but pylint’s failure to support this does not place a burden on us to support them.

@adamtheturtle

This comment has been minimized.

Copy link

commented Dec 23, 2017

@Lukasa - yes, I wrote that comment to try to be helpful for people coming to this issue with the same issue that I had.

The hack that I did as a good-enough-for-me workaround was to run:

import string

import requests

pattern = '(requests\.)?codes\.({allowed}),'
accepted_characters = set(string.ascii_letters + '_')
keys = [key for key in requests.codes.__dict__.keys() if set(key).issubset(accepted_characters)]
print(pattern.format(allowed='|'.join(keys)))

and then add the outputted string to my pylint-rc's generated-members section.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.