pep8.py does not catch import order violations #329

Closed
dcbaker opened this Issue Sep 23, 2014 · 10 comments

Comments

Projects
None yet
6 participants

dcbaker commented Sep 23, 2014

I've recently run into a case where I corrected the styling of a contributer to a project I work on, and he asked if there was a tool that could catch this error for him.

according to pep8:

Imports should be grouped in the following order:

  1. standard library imports
  2. related third party imports
  3. local application/library specific imports

You should put a blank line between each group of imports.

A simple test case looks like this:

import lxml
import itertools

The one caveat I see to implementing this feature is the common pattern of importing a fast alternative or falling back to the python builtin, like lxml or simplejson

try:
     import simplejson as json
execpt ImportError:
     import json
Contributor

florentx commented Sep 24, 2014

hello, there's an addition to flake8 which does this check: https://github.com/public/flake8-import-order

@florentx florentx added the extension label Dec 17, 2014

Owner

sigmavirus24 commented Oct 20, 2015

flake8-import-order does this very well. I think we can close this out for now.

vcarel commented May 12, 2016

flake8-import-order does this very well

I don't observe that on my projects. flake8-import-order mixes local modules imports with third-party imports.

Here is what pep8 recommends

Imports should be grouped in the following order:

standard library imports
related third party imports
local application/library specific imports

And here is what flake8-import-order asks me to do:

import hashlib
import os

from blueprints import oauth2, studies, upload, users
from conf import FLASK_SESSION_SECRET_KEY, OAUTH2_PROVIDER_URL
from environment import RESOURCE_DIR, VERSION
from flask import Flask, g, json, render_template
from flask.ext.jsonschema import JsonSchema
from mongo import client, ensure_indexes
from services.authentication import challenge_authentication, clear_session, get_auth_uri
from web_tools import redirect

Obviously flask is not a local package...

Member

jayvdb commented May 12, 2016

I also have not found any flake8 plugin which does this well. To get reasonable results I enable hacking and flake8-import-order, but disable some of each of their import rules.

Owner

sigmavirus24 commented May 12, 2016

I suspect that if you used absolute relative imports (e.g., from .conf import FLASK_SESSION_SECRET_KEY) flake8-import-order would catch that. You're making it far more explicit what is local and what isn't.

I agree that hacking in general does a better job, but hacking also requires you to install all of your third-party dependencies and the package itself for it to be able to determine what is third-party and what isn't (which in my honest opinion is subpar).

warsaw commented May 12, 2016

Despite PEP 8's recommendation (which I'm probably responsible for ;) I've mostly given up on trying to keep local from-imports separate from global from-imports. Here's the flake8 plugin I use to enforce my current rules on import order in Mailman 3:

https://gitlab.com/mailman/mailman/blob/master/src/mailman/testing/flake8.py

Owner

sigmavirus24 commented May 12, 2016

flake8's current branch of very active development is using flake8-import-order. I'll admit I'm not extremely satisfied with it, but I'm liking it better than hacking (for the reasons I enumerated above). That said, flake8-import-order has a concept of different conventions that it accepts (much like pydocstyle's notion) and I wonder if someone could hack together a convention that follows PEP-0008.

Until there's a good replacement for hacking, I'm fairly certain I'm going to stick with flake8-import-order to have some sort of convention around the imports in that project. It's genuinely better than nothing.

vcarel commented May 12, 2016

The problem on my side is that some of us use Intellij (which follows PEP8 recommendations), others don't. And even Intellij users do not optimize imports every time.

To not mess-up everybody, I was searching for a tool enforcing PEP8 recommendations, and nothing else.

@sigmavirus24 I agree our projects' structure is a bit weird. The "conf" module is resolved at runtime by PYTHONPATH. We use a Python file to store our configuration instead of a text file (or JSON or YML or whatever else).

Owner

sigmavirus24 commented May 12, 2016

@sigmavirus24 I agree our projects' structure is a bit weird.

I'm so sorry if what I wrote came across that way. That's not how I meant it at all. Let me try again? I was merely suggesting that instead of import local modules absolutely that you use explicit local imports which will be a really good idea if you move to Python 3. You can enforce that with from __future__ import absolute_import. I'm guessing that using those explicit local imports, flake8-import-order might be more useful.

vcarel commented May 12, 2016

@sigmavirus24 We already moved to Python 3. What is an explicit local import... ? Did ou mean "explicit relative import for local modules"

Please excuse my question... I confess I don't understand that well Python modules.

@dhood dhood referenced this issue in ament/ament_lint May 24, 2016

Closed

Python module import order not being checked #53

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment