Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#27173 closed Cleanup/optimization (wontfix)

Permit import statements to be longer than 80 characters

Reported by: Chris Jerdonek Owned by: nobody
Component: Core (Other) Version: 1.10
Severity: Normal Keywords:
Cc: Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Currently, Django's use of isort converts lines like this (81 characters):

from django.db.backends.mysql.base import DatabaseWrapper as MySQLDatabaseWrapper

to this:

from django.db.backends.mysql.base import \
    DatabaseWrapper as MySQLDatabaseWrapper

This looks uglier and is less convenient when searching (e.g. for occurrences of DatabaseWrapper in this example). It would be nice to configure isort to conform to Django's coding style of permitting up to 119 characters.

isort does seem to have a line_length setting, which would make this ticket easy to resolve.

Change History (8)

comment:1 by Chris Jerdonek, 8 years ago

Addendum: Django already has an isort section in its setup.cfg.

comment:2 by Tim Graham, 8 years ago

Component: UncategorizedCore (Other)
Resolution: wontfix
Status: newclosed
Type: UncategorizedCleanup/optimization

We discussed import line length when adding the isort configuration. My main concern is that it will make it difficult to sort imports manually as people who have a right margin at 80 characters would need to change it just for this. We prefer longer lines "when it helps readability". I don't think there's much difference in readability of imports, so I prefer to keep things simple.

You could mark those lines with # isort:skip to use a longer length but I don't see much advantage.

comment:3 by Chris Jerdonek, 8 years ago

My main concern is that it will make it difficult to sort imports manually as people who have a right margin at 80 characters would need to change it just for this.

I'm not sure I understand this argument. It seems like this just shifts the problem from "people who have a right margin at 80 characters" to "everyone" since now the lines that need to be sorted always get broken across more than one line, regardless of how one's right margin is set. Here is one example from the code base (in sessions_tests.tests):

from django.contrib.sessions.backends.cache import SessionStore as CacheSession
from django.contrib.sessions.backends.cached_db import \
    SessionStore as CacheDBSession
from django.contrib.sessions.backends.db import SessionStore as DatabaseSession
from django.contrib.sessions.backends.file import SessionStore as FileSession
from django.contrib.sessions.backends.signed_cookies import \
    SessionStore as CookieSession
from django.contrib.sessions.exceptions import InvalidSessionKey
from django.contrib.sessions.middleware import SessionMiddleware
from django.contrib.sessions.models import Session
from django.contrib.sessions.serializers import (
    JSONSerializer, PickleSerializer,
)
from django.core import management

comment:4 by Tim Graham, 8 years ago

I don't think longer import lengths will help readability except for the backslashes case. I don't mind changing those instances if the isort issue you created is fixed but rewriting all imports in Django to a new length is a no go for me. Maybe you don't realize that if we changed to [isort] line_length = 119, that would create a 6K line patch for Django.

comment:5 by Aymeric Augustin, 8 years ago

I agree with Tim, perhaps this would work around the backslash issue for 23 imports but it would also reduce readability for thousands of other imports.

comment:6 by Chris Jerdonek, 8 years ago

Good point re: the 6K patch (which didn't occur to me).

It's a shame that isort forces the code to be exactly "one way" as opposed to acting more as a linter and complaining only when there is nonconformance. That would permit incremental change (e.g. permitting the setting to change without having to make massive code changes) as well as not forcing Django to conform to buggy isort output.

Regarding readability, I agree except in those cases where a longer line would permit the import to fit on one line. If it's more than one line either way, then I agree there's little or no benefit.

comment:7 by Zach Borboa, 8 years ago

I hope I can sway your opinion.

Let's optimize for the reading case and not optimize to make code faster to write. After all, you'll write it once, but read it over and over and over.

  • Single-line import statements are the most simple to read. It also happens to create the cleanest diffs -- rather than wrapping long lines with a mixture of back slashes and parens which doesn't lend itself to quick scanning and overall just seems forced.

Is the "flat" import style of imports on separate lines an option? The previous example would be:

from django.contrib.sessions.backends.cache import SessionStore as CacheSession
from django.contrib.sessions.backends.cached_db import SessionStore as CacheDBSession
from django.contrib.sessions.backends.db import SessionStore as DatabaseSession
from django.contrib.sessions.backends.file import SessionStore as FileSession
from django.contrib.sessions.backends.signed_cookies import SessionStore as CookieSession
from django.contrib.sessions.exceptions import InvalidSessionKey
from django.contrib.sessions.middleware import SessionMiddleware
from django.contrib.sessions.models import Session
from django.contrib.sessions.serializers import JSONSerializer
from django.contrib.sessions.serializers import PickleSerializer
from django.core import management

comment:8 by Tim Graham, 8 years ago

We discussed this at the time of adding isort. I don't care to reopen the discussion as the current pattern is working well as far as I'm concerned, and I think we have more important issues to discuss.

Note: See TracTickets for help on using tickets.
Back to Top