Opened 10 years ago

Closed 9 years ago

#23395 closed Cleanup/optimization (fixed)

Clarification on PEP 8 E501: line too long (> 79 characters)

Reported by: nrogers64 Owned by: Amine Zyad
Component: Documentation Version: dev
Severity: Normal Keywords: afraid-to-commit
Cc: amizya@…, dodobas@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: yes UI/UX: no

Description

From the "Coding style" documentation:

One big exception to PEP 8 is our preference of longer line lengths. We’re well into the 21st Century, and we have high-resolution computer screens that can fit way more than 79 characters on a screen. Don’t limit lines of code to 79 characters if it means the code looks significantly uglier or is harder to read.

I have two issues with this:

  1. It says that you don't have a limit of 79 characters, but it doesn't say what you do instead. Is a line that is 1000+ characters acceptable?
  2. It seems to imply that this PEP 8 rule is outdated and that there is absolutely no reason whatsoever that anybody should follow it in a Django project these days. It's even worded in kind of a condescending manner, in my opinion. However, many developers prefer to have multiple files open side by side. For example, they might have a models.py and views.py file open side by side, or even multiple views of the same file open side by side. The limit of 79 characters is helpful here. I would suggest taking out the "21st Century" bit which seems to be there to justify Django not following this PEP 8 rule, but, as I've pointed out, it doesn't really justify it.

Attachments (4)

shorter-lines.diff (6.0 KB ) - added by Tim Graham 10 years ago.
cleanups-a.txt (46.7 KB ) - added by Tim Graham 10 years ago.
cleanups-b.txt (37.1 KB ) - added by Tim Graham 10 years ago.
cleanups-c.txt (37.6 KB ) - added by Tim Graham 10 years ago.

Download all attachments as: .zip

Change History (34)

comment:1 by Wim Feijen, 10 years ago

Easy pickings: set
Triage Stage: UnreviewedAccepted
Version: 1.6master

Hi nrogers64,

I agree with you that this text needs updating. As far as I know, the current guideline is actually to keep lines within 79 characters unless it doesn't fit, in which case it is ok to make longer lines. Clarity goes above following the character limit.

Maybe just delete that one sentence and make it:
One exception to PEP 8 is our opinion on line length. Don’t limit lines of code to 79 characters if it means the code looks significantly uglier or is harder to read.

comment:2 by Baptiste Mispelon, 10 years ago

Type: UncategorizedCleanup/optimization

I agree that the tone is too snarky and FYI, pep8 was recently amended regarding line lengths:

Some teams strongly prefer a longer line length. For code maintained exclusively or primarily by a team that can reach agreement on this issue, it is okay to increase the nominal line length from 80 to 100 characters (effectively increasing the maximum length to 99 characters), provided that comments and docstrings are still wrapped at 72 characters.

(http://legacy.python.org/dev/peps/pep-0008/#maximum-line-length)

comment:3 by Tim Graham, 10 years ago

Perhaps outside the scope of this ticket, but I'd like to propose adopting a max line length of 120 as that's the width of code review in GitHub. Anything longer requires horizontal scrolling which makes review more difficult. We could use flake8 to enforce this, however, it'll require a lot of cleanup as we currently have ~1700 lines longer than that. I'll attach an initial patch to show how long strings can be restructured.

by Tim Graham, 10 years ago

Attachment: shorter-lines.diff added

comment:4 by Collin Anderson, 10 years ago

I use 120 (119) characters personally, again, because of github.

comment:5 by Daniele Procida, 10 years ago

Keywords: afraid-to-commit added

I've marked this ticket as especially suitable for people following the ​Don't be afraid to commit tutorial at the DjangoCon US 2014 sprints. If you're tackling this ticket, please don't hesitate to ask me for guidance if you'd like any, either here or on the Django IRC channels, where I can be found as EvilDMP.

comment:7 by Tim Graham, 10 years ago

I'm working on the updates in django/ and I've split up the test updates into 3 files. Please claim one by commenting here if you want to work on it.

by Tim Graham, 10 years ago

Attachment: cleanups-a.txt added

by Tim Graham, 10 years ago

Attachment: cleanups-b.txt added

by Tim Graham, 10 years ago

Attachment: cleanups-c.txt added

comment:8 by nrogers64, 10 years ago

Tim, the only issue I have with your proposed wording at django-developers is this:

Documentation, comments, and docstrings should be wrapped at 79 characters.

As stated by bmispelon, PEP 8 says:

For code maintained exclusively or primarily by a team that can reach agreement on this issue, it is okay to increase the nominal line length from 80 to 100 characters (effectively increasing the maximum length to 99 characters), provided that comments and docstrings are still wrapped at 72 characters.

Also, I'm not sure if Carl is correct or not in this statement that he made:

With recent updates to PEP 8 to accommodate longer lines, the stated policy is no longer an exception to PEP 8

The quote from PEP 8 shown above seems to imply that you can go up to 99 characters if you want to, but no more than that. Maybe I'm misinterpreting that, though.

in reply to:  8 comment:9 by Carl Meyer, 10 years ago

Replying to nrogers64:

Also, I'm not sure if Carl is correct or not in this statement that he made:

With recent updates to PEP 8 to accommodate longer lines, the stated policy is no longer an exception to PEP 8

The quote from PEP 8 shown above seems to imply that you can go up to 99 characters if you want to, but no more than that. Maybe I'm misinterpreting that, though.

You're right; I didn't re-check the PEP and mis-remembered it as allowing up to 120, not 100. So allowing up to 120 remains an exception to PEP 8. Thanks for the correction.

comment:10 by Tim Graham, 10 years ago

I've updated the proposed wording in this PR which also fixes long lines in django/.

comment:11 by Tim Graham <timograham@…>, 10 years ago

In 1101467ce0756272a54f4c7bc65c4c335a94111b:

Limited lines to 119 characters in django/

refs #23395.

comment:12 by conor, 10 years ago

Hi guys, first-time contributor here. Am I right in saying that this patch has been applied and that this ticket can be closed? Sorry if I'm missing something.

comment:13 by Baptiste Mispelon, 10 years ago

There are still some lines longer than 120 characters, that's why this ticket still hasn't been closed.

You can find those lines by running flake8 --select=E501 (you need to install the flake8 package first).

comment:14 by Berker Peksag, 10 years ago

https://github.com/django/django/pull/3421

I left two instances untouched:

  • django/db/migrations/loader.py:115:120: E501 line too long (133 > 119 characters)
  • django/db/migrations/autodetector.py:432:120: E501 line too long (166 > 119 characters)

comment:15 by Berker Peksag, 10 years ago

Has patch: set

comment:16 by Tim Graham <timograham@…>, 10 years ago

In c9178ef17a0e1070f5a25096d8e13385d404dc92:

Limited lines to 119 characters in django/{contrib,db}.

Refs #23395.

comment:17 by Tim Graham <timograham@…>, 10 years ago

In d73c7e5db66b7c149db59e07bc4c6253629cf907:

[1.7.x] Limited lines to 119 characters in django/{contrib,db}.

Refs #23395.

Backport of c9178ef17a (to decrease chance of backport conflicts) from master

comment:18 by Tim Graham, 10 years ago

Has patch: unset

comment:19 by Joseph Leon, 10 years ago

Owner: changed from nobody to Joseph Leon
Status: newassigned

comment:20 by Joseph Leon, 10 years ago

Owner: Joseph Leon removed
Status: assignednew

comment:21 by Amine Zyad, 10 years ago

Cc: amizya@… added
Owner: set to Amine Zyad
Status: newassigned

I'll take care of this.

comment:22 by Tim Graham, 10 years ago

Resolution: fixed
Status: assignedclosed

Doesn't seem much value here in addressing the line lengths unless we can also exclude migration directories from being checked because the auto-generated code there often has very long lines. Excluding */migrations/* as done before 9d30412a5ad1c72b3d319b4c2bceacb53a0ff1da, however, prevents us from detecting other issues, especially in django/db/migrations. I don't think listing all migrations directories in flake8 exclude would be useful as an alternative, so unless anyone finds a solution, I think we can close this.

comment:23 by Tim Graham, 9 years ago

Resolution: fixed
Status: closednew

Let's try this using autopep8 to manually format migration files in order to limit line length. I am getting tired of commenting about line length on pull requests, so automated enforcement seems worthwhile.

comment:24 by Dražen Odobašić, 9 years ago

One way to deal with migrations is to exclude */0*.py which will not exclude django/db/migrations. The option also excludes some more esoteric migrations like ./tests/migrations/migrations_test_apps/conflicting_app_with_dependencies/migrations/0002_second.py and similar.

AFAICT, find -wholename '*/0*.py', no other files in Django repository, start with 0 and end with .py so it should be safe enough to use

Other than that, there are only 15 occurrences of E501 in migration files, 1018 problems remain :)

comment:25 by Tim Graham, 9 years ago

I think it can be useful to run flake8 checks on migrations -- especially when writing them manually as in the case of data migrations. So I think fixing those 15 cases either manually or automatically would be ideal.

comment:26 by Dražen Odobašić, 9 years ago

Cc: dodobas@… added

ok, so just to confirm we want to modify/refactor all lines that result with E501, in the Django master branch, right?

comment:27 by Tim Graham, 9 years ago

Correct. I'd suggest to start with the instances in django/ to get a feel for any style decisions. Then you can proceed to tests/. Thanks!

comment:29 by Tim Graham, 9 years ago

Has patch: set
Patch needs improvement: set

comment:30 by Tim Graham <timograham@…>, 9 years ago

Resolution: fixed
Status: newclosed

In b1e33ce:

Fixed #23395 -- Limited line lengths to 119 characters.

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