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

refactor: linewrapping comments to 100 width #1274

Merged

Conversation

distractedm1nd
Copy link
Collaborator

I reformatted all comments in the repository to wrap to 80 line width using https://github.com/alexkohler/cfmt.

@distractedm1nd
Copy link
Collaborator Author

Should we potentially add this to make fmt @Wondertan ?

MSevey
MSevey previously approved these changes Oct 24, 2022
Copy link
Member

@MSevey MSevey left a comment

Choose a reason for hiding this comment

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

Awesome 👏

Also +1 for adding this to make fmt

@walldiss
Copy link
Member

Love this being automated! Why 80 is chosen to be the value for line length? Diff seems to have many 1 word newlines so it seems there was some formatting before that used a higher value.

@MSevey
Copy link
Member

MSevey commented Oct 25, 2022

Love this being automated! Why 80 is chosen to be the value for line length? Diff seems to have many 1 word newlines so it seems there was some formatting before that used a higher value.

It actually dates back to punch cards I think haha but nowadays it it more about consistency and readability. Some languages recommend a maximum line length in their standards, see this short little article https://www.codereadability.com/maximum-line-length/

The comments bring up a number of nice points, like readability on laptops with split screens, accessibility concerns, etc.

@Wondertan
Copy link
Member

@MSevey, if the argument is about the split screen, should we then also stick to the code itself being no more than 80, not just comments like this PR and tool do? The article you sent argues about the code length AFAIS.

If we agree that we should shorten our code length, then we should also do it in this PR, imo

@MSevey
Copy link
Member

MSevey commented Oct 26, 2022

@MSevey, if the argument is about the split screen, should we then also stick to the code itself being no more than 80, not just comments like this PR and tool do? The article you sent argues about the code length AFAIS.

If we agree that we should shorten our code length, then we should also do it in this PR, imo

@Wondertan up to the team on line wrapping the code to 80. For me the difference is that the code has go fmt which ensures all code is formatted nicely and uniformly, where as comments were all over the place until this PR.

So for me it is less the number and more that there is something that is keeping things consistent. It just makes the code nicer to work with and you don't get PRs with huge formatting diffs because people have different formatting settings locally.

@Wondertan
Copy link
Member

because people have different formatting settings locally.

Good point.

The question on changing the code length number remains open for the team. Personally, I feel like the length of the comments should be the same as the code or just a little shorter. Like if the code is 120, the comments should be 100/120 and not 80. So the gap of 20 is acceptable, but not more than that. Otherwise it feels a bit odd to me.

@MSevey
Copy link
Member

MSevey commented Nov 3, 2022

The question on changing the code length number remains open for the team. Personally, I feel like the length of the comments should be the same as the code or just a little shorter. Like if the code is 120, the comments should be 100/120 and not 80. So the gap of 20 is acceptable, but not more than that. Otherwise it feels a bit odd to me.

yea, I'm not tied to 80, I'm just tied to consistency :-)

when some
comments are like this and have no consistent line length
but at least are wrapped


and others just yolo'd and never heard of line wrapping and were like, this is totally fine, I love scrolling right for days to finish reading a comment about how this method works but you know what I can't see the method any more and have to start scrolling back and forth now to see what this comment is describing.

it leaves me :triggered:

@Wondertan
Copy link
Member

Ok, so let's:

  • add this to make fmt in this PR
  • Try to add the same rule for the code itself
  • Rebase

And we are good to go

@distractedm1nd
Copy link
Collaborator Author

Set it to linelength 100 (code is at 120), and added it to make fmt.

One thing to note: I do not agree (and assume others are with me here) that the /minimum/ line length before a break should also be 100. It makes sense in a lot of places in our docs and explanation comments to line break earlier.

@distractedm1nd distractedm1nd changed the title refactor: linewrapping comments to 80 width refactor: linewrapping comments to 100 width Nov 9, 2022
renaynay
renaynay previously approved these changes Nov 9, 2022
Copy link
Member

@renaynay renaynay left a comment

Choose a reason for hiding this comment

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

ok thank u

@renaynay
Copy link
Member

renaynay commented Nov 9, 2022

@distractedm1nd minimum shouldn't be 100 tho

Makefile Show resolved Hide resolved
Wondertan
Wondertan previously approved these changes Nov 9, 2022
@MSevey MSevey self-requested a review November 9, 2022 18:55
MSevey
MSevey previously approved these changes Nov 9, 2022
@distractedm1nd distractedm1nd merged commit 793ec2c into celestiaorg:main Nov 9, 2022
@distractedm1nd distractedm1nd deleted the linewrapping-comments branch November 9, 2022 19:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants