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

feat: add wrap_comments and comment_width options to nargo fmt #7371

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

Conversation

asterite
Copy link
Collaborator

Description

Problem

Resolves #7332

Summary

This is an opt-in feature, just like in rustfmt.

Additional Context

I tried to make sure this covers most scenarios though I'm sure I missed some. We could enhance things as we find them.

I also thought about enabling comment wrapping in the stdlib and test programs (things become easier to read) but I'll leave that for an (optional) PR.

Question: should the default comment_width be 100 or 80? It's 80 in rustfmt, which is different than the default max_width of 100 and I don't know why. Maybe comments are easier to read if they are shorter (compared to code)?

Documentation

Check one:

  • No documentation needed.
  • Documentation included in this PR.
  • [For Experimental Features] Documentation to be submitted in a separate PR.

PR Checklist

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt on default settings.

@asterite asterite requested a review from a team February 13, 2025 20:14
@TomAFrench
Copy link
Member

How would you expect

// This is a long comment that's going to be wrapped.
// Short

to be formatted?

// This is a long comment
// that's going to be
// wrapped.
// Short

or 

// This is a long comment
// that's going to be
// wrapped. Short

@TomAFrench
Copy link
Member

We also don't push short comments onto a single line, e.g.

// This
// is
// a
// long

into

// This is a long

@asterite
Copy link
Collaborator Author

Right... I thought about those two cases. I actually wanted to see what rustfmt does but that's unstable and I was lazy to install an unsafe version of it to try it.

My guess is that this:

// This is a long comment that's going to be wrapped.
// Short

is formatted like this:

// This is a long comment
// that's going to be
// wrapped.
// Short

The main reason is that otherwise if you really want a newline after "wrapped." then the formatter would undo it (your second comment "We also don't push short comments onto a single line, e.g.")

This is especially tricky if code in comments is being formatted. If you write this:

// If we have a loop like this:
// 
// for i in 0..10 {
//   println("hello!");
// }

it would be kind of bad if that println always moves up because there's space.

I know it ends up a bit annoying that a wrapped comment might not look good, but my thought of this feature is that it makes sure you don't exceed the maximum, and the comment might end up not looking very nice and you'll fix it, always in a way that doesn't exceed the maximum.

One other thing: I think the feature in rustfmt is unstable because it has many bugs. I don't know if those bugs are because it's not clear how comments should be wrapped (this discussion).

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.

Add a wrap_comments option to nargo fmt
2 participants