Skip to content

fix(fmt): single line comments orphaned words overflow into next line #11132

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

0xheartcode
Copy link

Motivation

These two issues mentioned a bug with the forge fmt command: #3604 & #10173. Seemed like a good first-issue to get familiar with the codebase :)

Honestly doesn't seem like a bug to me, people might want to format things manually, but it's true it is not similar to how rustfmt.

Here was the situation before (from the ticket #3604):

Problem
Suppose you have some comments like this:

/// @notice Calculates the amount that the sender would be refunded if the stream were canceled, denoted in units
/// of the token's decimals.

I'm using a line_length of 120, so if I change denoted to denominated, like so:

/// @notice Calculates the amount that the sender would be refunded if the stream were canceled, denominated in units
/// of the token's decimals.

And then I run forge fmt, the following output will be produced:

/// @notice Calculates the amount that the sender would be refunded if the stream were canceled, denominated in
/// units
/// of the token's decimals.

As you can see, units has been pushed on line 2, and the rest of the comments have been pushed on line 3.

Requested Solution
Make forge fmt handle multiple lines of comments, and produce this output instead:

/// @notice Calculates the amount that the sender would be refunded if the stream were canceled, denominated in
/// units of the token's decimals.

The bug fix applies to the following situations:

  • wrap_comments = true
  • Only applies to Single line NatSpec type comments (///) that:
    1. Detects consecutive Single NatSpec comments that should be treated as logical units
    2. Respects semantic boundaries - won't merge across NatSpec tags (@) [not very strict here], markdown structure (-, *, >), numbered lists, or fully empty lines
    3. Applies strategic word distribution - merges text content and redistributes words optimally across lines to prevent orphaned words

Solution

Essentially we are overflowing if the condition mentioned above are true, and we have hit the line length limit.
So if line A: 82 and line B:2, line C:10.
After fmt line A:80 (overflow into B), line B:4, line C:10.

if line A: 82, line B: 82, line C:10.
After fmt line A:80 (overflow into B), line B:80 (overflow into C), C:14.

if line A: 82, line B: 2, line C:82, line D: 3.
After fmt line A:80 (overflow into B), line B:4 (we're safe!), C:80 (overflow into D), D: 5.

(of course these examples don't take into account the exact word length)
Maybe the naming should be changed ?

PR Checklist

  • Added Tests
  • Added Documentation
    Bug fix needs documentation?
  • Breaking changes
    No

@zerosnacks
Copy link
Member

Hi @0xheartcode thanks for your PR :)

I would propose we wait for #10907 to land and then rebase the proposed fix

@0xheartcode
Copy link
Author

Yes, was more of a suggestion really, as this was pretty old and pending. Please ping when it's good to rebase @zerosnacks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

2 participants