pylint-bot / pylint-unofficial

UNOFFICIAL playground for pylint github migration
0 stars 0 forks source link

wrong-import-order flagging stdlib modules as not stdlib #712

Open pylint-bot opened 8 years ago

pylint-bot commented 8 years ago

Originally reported by: Anthony Sottile (BitBucket: asottile, GitHub: @asottile?)


py27:

# pylint:disable=unused-import
import io
import os.path
$ ./.tox/py27/bin/pylint test.py 
************* Module test
C:  3, 0: standard import "import os.path" comes before "import io" (wrong-import-order)

py34:

# pylint:disable=unused-import
import json
import os.path
$ ./.tox/py34/bin/pylint test.py 
************* Module test
C:  3, 0: standard import "import os.path" comes before "import json" (wrong-import-order)

pylint-bot commented 8 years ago

Original comment by Anthony Sottile (BitBucket: asottile, GitHub: @asottile?):


fwiw, they're properly detected here: https://github.com/asottile/aspy.refactor_imports/blob/master/aspy/refactor_imports/classify.py

pylint-bot commented 8 years ago

Original comment by Claudiu Popa (BitBucket: PCManticore, GitHub: @PCManticore):


The message should really be improved, but what it says is that you should put json before os.path.

pylint-bot commented 8 years ago

Original comment by Florian Bruhin (BitBucket: The-Compiler, GitHub: @The-Compiler?):


To clarify, the checker checks if the imports are ordered alphabetically. I think it really should be turned off by default.

pylint-bot commented 8 years ago

Original comment by Anthony Sottile (BitBucket: asottile, GitHub: @asottile?):


They are alphabetical. Last I checked J / I comes before O.

Interestingly enough, the first example (that fails in py27), passes in py34.

pylint-bot commented 8 years ago

Original comment by Claudiu Popa (BitBucket: PCManticore, GitHub: @PCManticore):


Oups, you're right, J/I does indeed comes before O usually.

On another note, I can't reproduce it, not with 2.7, not with 3.5, so something must be amiss here.

Also, regarding what Florian said, if you folks don't find the value in these new messages (wrong-import-order, wrong-import-position and ungrouped-imports), if they are creating too much noise and consider that should be disabled by default, I think that we should consider this, at least transforming them into extensions which means that they have to be activated manually in order to be emitted. From my point of view, they aren't sources of problems, so I'm not against of disabling or moving them to the extension mechanism, which will make pylint a little less pedantic with regard to this area. Moving them in 1.5.1 sounds feasible to me.

pylint-bot commented 8 years ago

Original comment by Anthony Sottile (BitBucket: asottile, GitHub: @asottile?):


Seems related to the location of the stdlib. Seems pylint needs to be updated to look in /usr/lib64 for stdlib modules as well as /usr/lib?

(Checking trusty, they exist at /usr/lib -- on lucid (and perhaps others) they're in /usr/lib64)

pylint-bot commented 8 years ago

Original comment by Anthony Sottile (BitBucket: asottile, GitHub: @asottile?):


If you have docker at hand, here's a reproduction:

docker run -ti ubuntu:lucid bash -exc '
echo -e "deb http://ppa.launchpad.net/fkrull/deadsnakes/ubuntu lucid main\ndeb-src http://ppa.launchpad.net/fkrull/deadsnakes/ubuntu lucid main\n" >> /etc/apt/sources.list &&
apt-get update &&
apt-get install python2.7 python-pip -y --force-yes &&
pip install virtualenv &&
virtualenv venv -ppython2.7 &&
. venv/bin/activate &&
pip install pylint &&
echo -e "[REPORTS]\nreports=no\n" > pylintrc &&
echo -e "import io\nimport os.path\n" > test.py &&
pylint test.py
'

Output:

$ bash test.sh
+ echo -e 'deb http://ppa.launchpad.net/fkrull/deadsnakes/ubuntu lucid main\ndeb-src http://ppa.launchpad.net/fkrull/deadsnakes/ubuntu lucid main\n'
+ apt-get update
...
+ apt-get install python2.7 python-pip -y --force-yes
...
+ pip install virtualenv
...
+ virtualenv venv -ppython2.7
...
+ . venv/bin/activate
...
+ pip install pylint
...
  Downloading pylint-1.5.0-py2.py3-none-any.whl (539kB)
    100% |################################| 540kB 633kB/s 
...
+ echo -e '[REPORTS]\nreports=no\n'
+ echo -e 'import io\nimport os.path\n'
+ pylint test.py
************* Module test
C:  1, 0: Missing module docstring (missing-docstring)
W:  1, 0: Unused import io (unused-import)
W:  2, 0: Unused import os.path (unused-import)
C:  2, 0: standard import "import os.path" comes before "import io" (wrong-import-order)
pylint-bot commented 8 years ago

Original comment by Sylvain Thénault (BitBucket: sthenault, GitHub: @sthenault?):


It's probably because one is correctly detected as a module from the std lib while the other isn't.

This check isn't checking that imports are ordered alphabetically but according to pep8: 1. std lib 2. external libs 3. projects modules

Also it checks that imports from a same project are grouped together.

IMO this is a nice feature, so let's try to fix it before moving it to an extension.

pylint-bot commented 8 years ago

Original comment by Florian Bruhin (BitBucket: The-Compiler, GitHub: @The-Compiler?):


I see, I was just repeating what @PCManticore told me earlier :wink:

I think Claudiu and I were debugging something similar at EuroPython, with astroid's tests failing when run inside of a virtualenv:

https://bitbucket.org/logilab/astroid/issues/165/

pylint-bot commented 8 years ago

Original comment by Adrian Castravete (BitBucket: adrian-castravete, GitHub: @adrian-castravete?):


Has anyone else tried importing time and do the from __future__ import absolute_import ? They each in turn demand to be placed first.

Of course from __future__ import absolute_import should be placed first, but import time also triggers a wrong-import-error

pylint-bot commented 8 years ago

Original comment by Claudiu Popa (BitBucket: PCManticore, GitHub: @PCManticore):


This sounds similar to #725. We'll fix these in time for 1.5.2, hopefully we'll be able to release it at the end of the week.