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

Fixed #23860 -- Documented imports order convention. #3566

Closed
wants to merge 1 commit into from

Conversation

wrwrwr
Copy link
Contributor

@wrwrwr wrwrwr commented Nov 17, 2014

Please see #23860.

from django.views.generic.base import View

* Put imports in three groups: standard library, other Django packages, the
same package, as recommended by PEP; sort lines in each group alphabetically
Copy link
Member

Choose a reason for hiding this comment

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

Use :pep:XXX

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, any way to link to the imports section of the document?

@carljm
Copy link
Member

carljm commented Nov 17, 2014

This LGTM, modulo adding a blank line after __future__ imports.

@carljm
Copy link
Member

carljm commented Nov 17, 2014

@timgraham You do most of the code-formatting-enforcement these days (or so it seems to me, anyway) -- does this look OK to you?

* Break long lines using parentheses, indenting continuation lines by 4 spaces.
For example::

from __future__ import nested_scopes
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: The examples are a mix of Python 2 and Python 3 stdlib. nested_scopes is too old. I'd replace it with something like print_function. Also, concurrent.futures is only available on Python 3 stdlib. I'd replace it with email.message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, both replaced. nested_scope was the first one introduced and I believe all are guaranteed to be available forever, but the one that Django mostly uses nowadays is unicode_literals.

wrwrwr added a commit to wrwrwr/django that referenced this pull request Nov 21, 2014
import bcrypt

from django.contrib.messages import MessageFailure, ERROR, error
from django.db import (models, transaction, DataError, DatabaseError, Error, IntegrityError,
Copy link
Contributor

Choose a reason for hiding this comment

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

from django.db import (
    models, transaction, DataError, DatabaseError, Error, IntegrityError,
    InterfaceError, InternalError, NotSupportedError, OperationalError,
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd leave the closing brace on the 3rd line.

Ending the first line after the opening parenthesis is currently more or less as common as starting to list objects right away (173 cases compared to 152, but 88 files versus 124. Making a line break before the closing parenthesis isn't that popular however, with 60 files versus 148.

I can't guarantee the full accuracy of these stats (if any at all ;-), some of the commands used to get the numbers:

  • grep -RPzl 'from\s+[\w.]+\s+import\s+\(' --include=*.py . | wc -l (205 files in total)
  • grep -RPzl 'from\s+[\w.]+\s+import\s+\(\s*(\n|#)' --include=*.py . | wc -l (88)
  • grep -RPzl 'from\s+[\w.]+\s+import\s+\(\w' --include=*.py . | wc -l (124)
  • grep -RPzl 'from\s+[\w.]+\s+import\s+\([^\)]*\n\)' --include=*.py . | wc -l (60)
  • grep -RPzl 'from\s+[\w.]+\s+import\s+\([^\)]*\w\)' --include=*.py . | wc -l (148)

What do you think?

@berkerpeksag
Copy link
Contributor

"Fixed #23860 -- Document [...]" -> "Fixed #23860 -- Documented [...]"


* Put imports in five groups: future, standard library, third-party libraries,
other Django components, the same component; sort lines in each group
alphabetically by the full module name. Place ``import module`` statements
Copy link
Member

Choose a reason for hiding this comment

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

Do we really place import ... after from .. import ...? I think we mix them right now and just sort them by module.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've written this fragment choosing a style at random (just looking at a few files), actually changing it from "before" to "after" in an early update. Having them sorted out seemed slightly more elegant, but no real arguments on this one.

An imperfect look at the sources:

  • a single block with from ... lines followed by some import ... lines and then from ... again: 20 files;
  • one or more import lines followed by two or more from lines: 48 (some of these are import django at the beginning of Django imports or are alphabetical by a different coincidence, some others are due to a missing blank line after the standard library block);
  • at least two from lines before an import line: 17 (again some of these are alphabetical).

Here are the commands I've used, maybe you'll figure out something more accurate:

  • grep -RPzl '\nfrom[^\n]+\n(import[^\n]+\n)+from' --include=*.py . | wc -l (20)
  • grep -RPzl '\n(import[^\n]+\n)+(from[^\n]+\n){2,}' --include=*.py . | wc -l (48)
  • grep -RPzl '\n(from[^\n]+\n){2,}import' --include=*.py . | wc -l (17)

(note that you may drop the wc -l and replace l with o to see the matches in all of these).

I'm not sure if isort has a setting for this one -- it seems it puts import lines before from lines.

@wrwrwr wrwrwr changed the title Fixed #23860 -- Document imports order convention. Fixed #23860 -- Documented imports order convention. Jan 17, 2015
@timgraham
Copy link
Member

I've resumed work on this with isort in #4008 and #4009. Thanks for starting the conversation.

@timgraham timgraham closed this Jan 29, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants