#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 , 8 years ago
comment:2 by , 8 years ago
Component: | Uncategorized → Core (Other) |
---|---|
Resolution: | → wontfix |
Status: | new → closed |
Type: | Uncategorized → Cleanup/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 , 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 , 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 , 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 , 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 , 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.
- A large 6K+ diff is not unheard of. (https://github.com/django/django/commit/c14937cf7a1e8c25702e89485cc2dd33aa0d3a16.diff).
- 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 , 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.
Addendum: Django already has an isort section in its
setup.cfg
.