Opened 2 months ago

Closed 2 months ago

Last modified 8 weeks ago

#36500 closed Bug (fixed)

inconsistent 79 char limit for docstrings and comments

Reported by: Mike Edmunds Owned by: Mike Edmunds
Component: Core (Other) Version: dev
Severity: Normal Keywords: flake8
Cc: David Smith Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Mike Edmunds)

Django's coding style has long required limiting docstrings and comments to 79 characters, while allowing a larger 88-char limit for code lines. (The latter matches Black's default.)

But a large number of files in Django's source code have docstrings and block comments with lines that are between 80–88 characters long, violating this coding standard.

Currently, only the 88 char limit is enforced by pre-commit linting, via flake8. The 79 char limit is (sometimes) manually enforced during PR review.

Early versions of flake8 supported only a single max-line-length limit that applied to both code and comments/docstrings. flake8 3.7.8 (2019-07-08) added a separate max-doc-length configuration option.

Django should either automatically enforce (via flake8 and pre-commit) this requirement or remove it so reviewers and contributors don't need to spend time cycling on line length.

[Edited to clarify discrepancy between existing coding standard and existing code.]

Change History (15)

comment:1 by Mike Edmunds, 2 months ago

Has patch: set

comment:2 by Jacob Walls, 2 months ago

Cc: David Smith added
Component: PackagingDocumentation
Version: 5.2dev

To follow up on a question asked in the PR, the existing files are fixed in https://github.com/django/django/pull/19549. There was some discussion about pre-commit hooks there as well. David, what do you think of this proposal -- can flake8 handle the complexity I see in check_line_too_long_django() in your PR?

comment:3 by Mike Edmunds, 2 months ago

Component: DocumentationUncategorized

This issue is about the max line length for triple-quoted """docstrings""" and # comment lines in .py files in the django/ and tests/ directories. (The PR Jacob linked is for reStructuredText .txt files in the docs/ directory, which have a similar length restriction but are otherwise unrelated to this issue. I'm not sure what the correct category is, but it's not "Documentation".)

Right now, Django's Linters CI job and pre-commit checks enforce only the 88-char overall limit in .py files. The smaller 79-char limit for docstrings and comments is enforced manually during PR review, if the reviewer happens to notice it. This creates extra hurdles and noise for both reviewers and contributors.

Unfortunately, the limit for docstrings and comments hasn't been consistently enforced, and there are over 1300 cases where Django's existing .py code doesn't comply with its own style requirements (see https://github.com/django/django/pull/19627#issuecomment-3050890027).

So setting flake8 max-doc-length = 79 would require fixing all those exceptions first, or grandfathering them in (e.g., by listing the problem files in .flake8 per-file-ignores).

An alternative might be adding args: ["--max-docs-length=79"] to the flake8 section of pre-commit-config.yaml. That would make pre-commit enforce the limit on modified files, without needing to address all the existing violations. The downside is it only works for people who have configured pre-commit, and the CI Linters job wouldn't catch the problem.

Version 0, edited 2 months ago by Mike Edmunds (next)

comment:5 by Jacob Walls, 2 months ago

Appreciate the clarification: that was hasty of me to link that PR, as well as to not check for related tickets first.

Previous discussion about auto-fixers for docstrings can be found #25263 (wontfix/Documentation). (Core(Other) also seems reasonable, since we have other flake8 tickets there.) It might make sense to treat this as a duplicate like #27668 pending the forum discussion (thanks for starting one!)

An alternative might be adding args: --max-docs-length=79 to the flake8 section of pre-commit-config.yaml (without changing the global .flake8 config). That would make pre-commit enforce the limit on modified files, without needing to address all the existing violations.

I doubt this would be workable, as it would format the whole file. (To me, the practice of avoiding unrelated cosmetic changes is more important than the line length.)

comment:6 by Sarah Boyce, 2 months ago

Resolution: wontfix
Status: assignedclosed

Given this is currently being discussed, we can reopen the ticket once we've come to a conclusion

comment:7 by Natalia Bidart, 2 months ago

Component: UncategorizedCore (Other)

comment:8 by Mike Edmunds, 2 months ago

Description: modified (diff)
Keywords: flake8 added
Resolution: wontfix
Status: closednew
Summary: pre-commit should enforce 79 char limit for docstrings and commentsinconsistent 79 char limit for docstrings and comments
Type: Cleanup/optimizationBug

The consensus in the forum discussion seems to be to remove the separate 79-char limit for docstrings and comments.

There's also a strong argument for keeping the requirement and bringing the existing code in compliance so it can be enforced by flake8.

I've opened PRs for both options. (The second one is, naturally, quite a bit more complicated. It would require some additional manual reformatting and careful review/merge coordination.)

comment:9 by Natalia Bidart, 2 months ago

Triage Stage: UnreviewedAccepted

Thank you Mike!

comment:10 by Natalia Bidart, 2 months ago

Triage Stage: AcceptedReady for checkin

comment:11 by nessita <124304+nessita@…>, 2 months ago

In 55b0cc21:

Refs #36500 -- Shortened some long docstrings and comments.

Manually reformatted some long docstrings and comments that would be
damaged by the to-be-applied autofixer script, in cases where editorial
judgment seemed necessary for style or wording changes.

comment:12 by nessita <124304+nessita@…>, 2 months ago

In 69a93a8:

Refs #36500 -- Rewrapped long docstrings and block comments via a script.

Rewrapped long docstrings and block comments to 79 characters + newline
using script from https://github.com/medmunds/autofix-w505.

comment:13 by nessita <124304+nessita@…>, 2 months ago

In 78298b5:

Refs #36500 -- Corrected rewrapped long lines fixed via a script.

Manually reformatted some comments and docstrings where autofix_w505.py
changed the meaning of the formatting.

comment:14 by nessita <124304+nessita@…>, 2 months ago

Resolution: fixed
Status: newclosed

In 3ad0e759:

Fixed #36500 -- Set flake8 max-doc-length config to 79 columns.

Set flake8 max-doc-length to 79 to enforce smaller line length limit
on docstrings and comments (per coding-style docs).

Updated docs to clarify both requirements are enforced by flake8 and
to remove some leftover language from the pre-black era.

comment:15 by GitHub <noreply@…>, 8 weeks ago

In f96c8f07:

Refs #36500 -- Ignored formatting changes in git blame.

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