[Django] #27173: Permit import statements to be longer than 80 characters
Django
-------------------------------+--------------------
Reporter: cjerdonek | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 1.10
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------
Currently, Django's use of [https://github.com/timothycrosley/isort isort]
converts lines like this (81 characters):
{{{#!python
from django.db.backends.mysql.base import DatabaseWrapper as
MySQLDatabaseWrapper
}}}
to this:
{{{#!python
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
[https://docs.djangoproject.com/en/dev/internals/contributing/writing-code
/coding-style/#python-style Django's coding style] of permitting up to 119
characters.
isort [https://github.com/timothycrosley/isort#configuring-isort does seem
to have] a `line_length` setting, which would make this ticket easy to
resolve.
--
Ticket URL: <https://code.djangoproject.com/ticket/27173>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: cjerdonek | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 1.10
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by cjerdonek):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
Addendum: Django already has an
[https://github.com/django/django/blob/9a2a52558e080f109a27d40a033a135c9d0e7e50/setup.cfg#L10
isort section] in its `setup.cfg`.
--
Ticket URL: <https://code.djangoproject.com/ticket/27173#comment:1>
Django
Reporter: cjerdonek | Owner: nobody
Type: | Status: closed
Cleanup/optimization |
Component: Core (Other) | Version: 1.10
Severity: Normal | Resolution: wontfix
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* status: new => closed
* resolution: => wontfix
* component: Uncategorized => Core (Other)
* type: Uncategorized => Cleanup/optimization
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/27173#comment:2>
Django
Reporter: cjerdonek | Owner: nobody
Type: | Status: closed
Cleanup/optimization |
Component: Core (Other) | Version: 1.10
Severity: Normal | Resolution: wontfix
| Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by cjerdonek):
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`):
{{{#!python
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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/27173#comment:3>
Django
Reporter: cjerdonek | Owner: nobody
Type: | Status: closed
Cleanup/optimization |
Component: Core (Other) | Version: 1.10
Severity: Normal | Resolution: wontfix
| Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
I don't think longer import lengths will help readability except for the
backslashes case. I don't mind changing those instances if the
[https://github.com/timothycrosley/isort/issues/466 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/27173#comment:4>
Django
Reporter: cjerdonek | Owner: nobody
Type: | Status: closed
Cleanup/optimization |
Component: Core (Other) | Version: 1.10
Severity: Normal | Resolution: wontfix
| Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by aaugustin):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/27173#comment:5>
Django
Reporter: cjerdonek | Owner: nobody
Type: | Status: closed
Cleanup/optimization |
Component: Core (Other) | Version: 1.10
Severity: Normal | Resolution: wontfix
| Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by cjerdonek):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/27173#comment:6>
Django
Reporter: cjerdonek | Owner: nobody
Type: | Status: closed
Cleanup/optimization |
Component: Core (Other) | Version: 1.10
Severity: Normal | Resolution: wontfix
| Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by zachborboa):
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:
{{{#!python
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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/27173#comment:7>
Django
Reporter: cjerdonek | Owner: nobody
Type: | Status: closed
Cleanup/optimization |
Component: Core (Other) | Version: 1.10
Severity: Normal | Resolution: wontfix
| Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
We discussed this [https://groups.google.com/d/topic/django-
developers/EQhbI4cDuX4/discussion 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/27173#comment:8>