Enforce or eliminate comment/docstring line length limit

Django Internals
1

Django’s coding style requirements have two different maximum line lengths for .py files, one for code lines and another for """docstrings""" and # comment lines:

We allow up to 88 characters as this is the line length used by black. This check is included when you run flake8. Documentation, comments, and docstrings should be wrapped at 79 characters, even though PEP 8 suggests 72.

Only the 88 char limit is enforced by Django’s flake8 config—including pre-commit hooks and the CI linters job. Currently, the smaller 79 char limit is (sometimes) manually enforced in the PR review process. This seems to me a poor use of both reviewers’ and contributors’ time and efforts.

I created ticket-36500 and PR #19627, thinking this could be quickly solved by adding max-doc-length = 79 to the .flake8 config file. Sadly, it’s not so simple. There are well over 1300 cases of existing code that exceeds that limit, across 380 files (roughly 14% of the .py files in the codebase). Thus, this post.

Some options:

  1. Use black’s 88-char limit for all code. Remove the separate 79-char limit on docstrings/comments from the coding standards. (All existing code is then already compliant.)
  2. The docs say “should” not “must.” Don’t treat 79 chars as a hard limit, and ignore it in PR reviews.
  3. Add max-doc-length = 79 to the flake8 config, and grandfather in the existing violations using per-file-ignores (like this). Clean up existing violations as the code is later edited for other reasons.
  4. Clean up the existing violations first, then change the flake8 config. (I’m not volunteering.)
  5. Add args: ["--max-docs-length=79"] to the pre-commit flake8 config—but not to the project-wide .flake8 config file. This would catch new edits that exceed the limit, in pre-commit only (for contributors who have pre-commit set up). But it wouldn’t check the limit in the CI linters job, and enforcement in PRs would still be manual. [Edit: this is probably not workable. See Jacob Walls’ comment in the ticket.]

My preference would be either option 1 or 3. (Or option 4 if someone else is up for the work.) My feeling is that purely mechanical style requirements should either be enforced automatically, or should be eliminated if they can’t. (After all, that’s why we use tools like flake8 and black.)

1 Like
2

I didn’t even know there was a 79 char limit on docstrings lol… Nobody’s ever flagged that with me.

I’d vote for 1 so we can get back to more important things :grinning_face_with_smiling_eyes:

1 Like
3

I’d also prefer to go with option 1, I’d bet not so many people are still using a fixed 80 char width terminal when looking at docs.

4

I’m +1 for 1. I stopped enforcing max line lengths in most projects. Black and Ruff wrap when they can, and when they can’t, it’s usually for something that’s hard to split, like a very long URL.

1 Like
5

I think it is also to do with line length readability, the optimal line length for text being 50–75 characters, PEP-8 assumes comments/docstrings are more like text than code. I think that is what informed their 72 limit and django’s compromise at 79. I still don’t think I would be in favour of enforcing it though.

I did take a few minutes to generate a gist of all the offending comments. A quick scan of that says that you could automate updating most of these, most just need wrapping, there are a few with long links you could allow, but you’d see a few, eg 1 & 2 that you’d want a more considerate approach on, so finding them is a bit of a chore. And any PR review would be an undertaking too.

Maybe updating the style guide to 88 with some language about trying to keep long block comments readable. Where ultimately language choice will have more impact on readability than line length ever would in any case.

1 Like
6

Another readability concern is being able to see side-by-side diffs without horizontal scrolling. I suspect the difference between 79 and 88 chars isn’t significant, but we should probably check with the Django Fellows (and anyone else who spends a lot of time looking at diffs).

:+1: There’s also a notion of splitting lines semantically—at natural breaks—rather than trying to fill available width. This can help with both readability and maintainability. Related Python forum discussion from 2022. (Those with print media backgrounds might recognize “semantic linebreaks” as a common guideline for wrapping titles and headlines.)

1 Like
7

Splitting lines semantically is usually why I tend to violate line length for comments, good to see this being acknowledged :+1:

8

I configure my text editor to display a right margin at 80 characters to help me wrap docstrings, comments, and code there. I know I have a few extra characters in the case of code if it helps to make a more semantic break. Sometimes I’m mildly annoyed that black reflows my 80 character wrapped code to fill the full 88 character limit, but whatever.

As a fellow, I never felt that wrapping docstrings and comments was a large burden. To that end, I think option 3 (adding max-doc-length = 79 but ignoring existing files with violations) is an easy win.

Option 1 feels more disruptive. For example, if the new docstring length is 88 characters and I’m making a small edit to an existing docstring wrapped at 80 characters, do I reflow the entire thing to 88? That seems needless and will cause a lot of inconsistency. If this new policy goes into effect, presumably I would change my editor’s right margin to 88 (which would affect more of my work that just Django). But without a right margin at 80, it would be difficult to continue wrapping existing docstrings at that length.

1 Like
9

I prefer to go with option 1.

(In my opinion 88 chars per line is too short anyway, but that’s a different topic.)

1 Like
10

I hope that’s not how it would be handled. The intent is to allow up to 88 characters in any line (code or text) in a .py file, but not require using all of it. We’d encourage text to be wrapped naturally for readability and maintainability, not mandate it fill the maximum space.

If you’re making a small edit, I think you’d have the flexibility to re-wrap the docstring or not, whichever results in more readable text or a clearer diff.

I had done the same thing. But Django’s limit is actually 79 chars, not 80. If you have punctuation in column 80, that won’t (well, might not) pass review. So now I have a project-specific override for Django that displays guides at 79 (and 87) chars.

Even with that, I still want flake8/pre-commit/CI to catch cases I overlook, to avoid spending scarce (human) review cycles on line-length concerns. (Also, if a reviewer leaves a large number of line-wrapping requests, it can be easy to overlook their more substantive feedback. Particularly given GitHub’s opinionated stance that when a review includes more than ~6 comments, some of them just don’t matter.)

Incidentally, I suspect the current formatting standard is left over from the pre-black days when Django allowed code up to 119 characters. For text, 119 chars is genuinely less readable than 79, and a smaller docstring/block comment line length would be important. Now that code is limited to 88 chars, the distinction from 79 seems less material.

1 Like
11

I’m unsure that option 1 will eliminate all review comments and thinking about line length. So some people may wrap their docstrings at 80 while some might wrap at 88? I was thinking you had me convinced that I could change my right margin to 88 and be fine with option 1, but then I realized I still need to wrap documentation source files at 80, thus I think I would still wrap docstrings there too (since I don’t want to be changing my margin for each source file type). Having docstrings routinely extend past my right margin would be unsightly for this neat freak. (You can tell me, “Get over it, TIM!” if you think I’m being pedantic.)

1 Like
12

This is exactly what I do. In my head, the limit is 80cols (including newline), for :sparkles: EVERYTHING :sparkles:

I can go a little over that limit for code when it’s worth it, but when “seeing the Matrix”, it’s wrapped at 80cols :rabbit_face: To me, needing longer lines is usually (but not always) a potential code smell.

I absolutely agree with this, and I want to help pushing something in this front.

I would definitely not go with option 2, nor 3, nor 5. I prefer 4, I think we could wrap the existing offending lines with a script or tool (I think ruff could be used with some params tweaks, or docformatter maybe?), do a single commit and “hide” that commit (like we have done with black, and just like are planning to do with the docs linter). I feel this is more consistent in terms of “text remains at 80cols” (both for docs and code, including newline).

It’s worth noting that there is also an option 6: enforce 80cols (including newline) for code as well, having a consistent limit across the whole code base, .py and docs files. This is easily doable by changing the line length config in black and reformatting in one go! :star2:

13

/me quietly weeps recalling the discussion around the transition to black

(Entire benefit was seen to be in that there are no config options to debate about.)

2 Likes
14

After 12 days, it seems like the consensus here is for option 1.

I’m going to reopen ticket-36500 as a request for Django’s code to match its coding standards. I’ve also opened two competing PRs:

  • PR#19660 implements option 1: make the standards match existing code by removing the separate 79-char limit on docstrings/comments
  • PR#19661 implements option 4: make the code match the standards by running an autofixer script (more about that below)

I defer to the Django Fellows’ judgment on which they’d prefer.

That’s the situation we already have. The existing code is a mixture of both lengths. There are several cases where the contributor (and reviewers) clearly believed the limit was 88, not 79, such as this carefully formatted table in a comment. Or this reST table in a docstring (which gets parsed at runtime by the admindocs package, causing confusing “Unexpected section title or transition” errors if you break the formatting).

I think it’s a bug that Django’s code doesn’t match Django’s required coding style. The discussion here has convinced me the only workable fixes are changing the requirements to match the code (option 1) or the code to match the requirements (option 4).

(The other options don’t seem workable: option 2 is vague; option 3 would skip enforcement in a seemingly random 1/7 of the source files; option 5 would mandate any future patches to those files include unrelated reformatting first. @nessita’s option 6 would require the same docstring/comment reformatting as option 4, so I’m treating them as equivalent here.)

I looked into this. ruff can’t autofix W505 doc-line-to-long errors. docformatter only does docstrings, not comments. (Also, it wants to rewrap every docstring, not just ones with long lines. I did a quick test run. Even if it could do comments, docformatter would produce a huge diff and would require a lot of manual cleanup.)

I used some spare credits to have an AI put together an autofixer[1] and (eventually) came up with autofix_w505.py. My goal was to minimize diffs by handling some common patterns in existing Django docstrings/comments. The PR linked above includes that. Looking through it, I saw a dozen or so places where we’d want some manual reformatting before merging.

That PR will go stale quickly. As @blighj pointed out earlier, review will be non-trivial. But if it’s the preferred fix, I’m willing to do the manual edits. (But only once. We’d need to coordinate a fast-track review and merge so the manual work doesn’t get outdated.)

  1. And the AI did a terrible job. Just awful. Despite my providing a detailed brief and test cases, JetBrains Junie v251.204.104 in “think more” mode generated buggy, repetitive code that didn’t come close to meeting the spec or passing the tests. When I pointed out the failing tests and asked it to fix the script, it modified the tests instead. (To ignore leading whitespace! Which still didn’t make them pass!) I ended up keeping the skeleton of the script but gutting it and rewriting all the functional parts by hand. (In fairness to LLMs, Claude 4 Sonnet did then provide a helpful review of my code, and even caught a bug.) ↩︎

1 Like
15

Thank you @medmunds for your effort here. :trophy:

I agree the PR that fixes comments is merge-sensitive so I’ll take a look today or tomorrow.

16

One other drawback to option 4 is it’s likely to cause merge conflicts in a large number of open PRs. (But I guess we went through that when switching to black, and this shouldn’t be anywhere near as disruptive as that was.)