Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

72 char docstrings #72

Merged
merged 1 commit into from
Aug 23, 2022
Merged

72 char docstrings #72

merged 1 commit into from
Aug 23, 2022

Conversation

Simon-1-1
Copy link
Contributor

Make doc strings PEP8 compliant.

@Simon-1-1 Simon-1-1 requested a review from plaos as a code owner August 10, 2022 12:34
@plaos
Copy link
Contributor

plaos commented Aug 11, 2022

I'm actually quite hesitant regarding this. I do like it for the doc comments, but I really don't like that it forces all comments to be 72 characters. Is there any way at all we could make it so that only doc comments are affected?

@Simon-1-1
Copy link
Contributor Author

I don't mind it for ordinary comments.

There is no way to configure it to not go for ordinary comments. But you could fork flake8-docstrings. It's just a plugin to flake8

@plaos
Copy link
Contributor

plaos commented Aug 11, 2022

Maybe, but I don't think we'll get around to that.

But what's your thought on the scenarios in which you have a method in a class, and a few nested loops/if-statements? The cases in which you only have something like 10-20 characters to write a comment? I think that harms readability more than the 72 character limit helps it for doc comments.

@Simon-1-1
Copy link
Contributor Author

I think that it keeps us in check so that we don't have 10 levels of nesting

@Simon-1-1
Copy link
Contributor Author

So what do you think?

@plaos
Copy link
Contributor

plaos commented Aug 16, 2022

I'm still not sold on it, I don't think the allowed width of a comment will keep us in check regarding multiple indentations. Also it seems a bit confusing to have different limits for code and comments, wouldn't it be difficult to have editor support for it? Do you have any arguments for why we want this?

@Simon-1-1
Copy link
Contributor Author

I'm still not sold on it, I don't think the allowed width of a comment will keep us in check regarding multiple indentations. Also it seems a bit confusing to have different limits for code and comments, wouldn't it be difficult to have editor support for it? Do you have any arguments for why we want this?

Yes, PEP8 requires it:

For flowing long blocks of text with fewer structural restrictions (docstrings or comments), the line length should be limited to 72 characters.

https://peps.python.org/pep-0008/#maximum-line-length

@plaos
Copy link
Contributor

plaos commented Aug 19, 2022

Seeing as it actually is in the PEP standard, I'm inclined to fold. I'll take another look at the changes.

Copy link
Contributor

@plaos plaos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall.

.github/workflows/check.yml Outdated Show resolved Hide resolved
reviewcheck/config.py Show resolved Hide resolved
According to PEP8, doc strings should be 72 characters wide at most.
They now are.
@plaos plaos merged commit c4a3f71 into main Aug 23, 2022
@plaos plaos deleted the 72-char-docstrings branch August 23, 2022 12:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants