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

[unstable option] comment_width #3349

Open
Tracked by #9
scampi opened this issue Feb 13, 2019 · 8 comments
Open
Tracked by #9

[unstable option] comment_width #3349

scampi opened this issue Feb 13, 2019 · 8 comments
Labels
unstable option tracking issue of an unstable option

Comments

@scampi
Copy link
Contributor

scampi commented Feb 13, 2019

Tracking issue for unstable option: comment_width

@scampi scampi added the unstable option tracking issue of an unstable option label Feb 13, 2019
@scampi scampi changed the title [unstable option] unstable option: comment_width [unstable option] comment_width Feb 18, 2019
@carols10cents
Copy link
Member

Not sure if this is the right place to add this comment or not; feel free to tell me to go somewhere else.

I was surprised to find out that the default value for comment_width is 80, while the default value for max_width is 100. I would have expected the defaults to be the same.

So before comment_width is stabilized, I'd like to advocate for reconsidering the default value.

Do text editors display/enforce different widths for comments vs code? I use TextMate, and it just has one wrap column setting.

Examples:

What looks "good" to me:

// This line of code is just about width 100
fn evaluate_something_test(something: &Something, number: usize) -> Result<BTreeMap, CustomError> {
    // This comment is wrapped to 100. Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed 
    // do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis 
    // nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute...
    unimplemented!();
    
    // This is what I would expect. irure dolor in reprehenderit in voluptate velit esse cillum 
    // dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in 
    // culpa qui officia deserunt mollit anim id est laborum.
}

What looks "strange" to me:

// This line of code is just about width 100
fn evaluate_something_test(something: &Something, number: usize) -> Result<BTreeMap, CustomError> {
    // This comment is wrapped to 80. Lorem ipsum dolor sit amet, consectetur 
    // adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore 
    // magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation 
    // ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute...
    unimplemented!();
    
    // This looks strange to me. irure dolor in reprehenderit in voluptate 
    // velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint 
    // occaecat cupidatat non proident, sunt in culpa qui officia deserunt 
    // mollit anim id est laborum.
}

mnmaita added a commit to mnmaita/bevy that referenced this issue Mar 9, 2021
bors bot pushed a commit to bevyengine/bevy that referenced this issue Mar 10, 2021
Aims to close #1594.

These options are unstable and depend on the following PR's:

[wrap_comments](https://rust-lang.github.io/rustfmt/?version=v1.4.36&search=#wrap_comments): rust-lang/rustfmt#3347

[comment_width](https://rust-lang.github.io/rustfmt/?version=v1.4.36&search=#comment_width): rust-lang/rustfmt#3349

[normalize_comments](https://rust-lang.github.io/rustfmt/?version=v1.4.36&search=#normalize_comments): rust-lang/rustfmt#3350

@alice-i-cecile do you think this will solve the issue? When enabled, running the formatter locally should take the configurations into account to format comments. `--check` runs should also be considering them. This should be testable on the `nightly` toolchain.

~I didn't delve into normalizing `//` vs `/* */` though, should I take a look into that too? [normalize_comments](https://rust-lang.github.io/rustfmt/?version=v1.4.36&search=#normalize_comments) seems to be the solution for that but it's also unstable (tracking issue: rust-lang/rustfmt#3350). I can also add this configuration (commented out, of course) if it's desirable.~ Added `normalize_comments` option.
luckysori added a commit to get10101/itchysats that referenced this issue Jul 28, 2022
It was a bit weird that the `comment_width` was configured to be
_longer_ than the `max_width` which sets the maximum width of each
line.

I choose to explictly set `max_width` to its default value of 100 for
clarity.

The reason why we don't see any other changes to the diff should be
obvious: setting `comment_width` to more than `max_width` was useless!

Inspired by:
rust-lang/rustfmt#3349 (comment).
luckysori added a commit to get10101/itchysats that referenced this issue Jul 28, 2022
It was a bit weird that the `comment_width` was configured to be
_longer_ than the `max_width` which sets the maximum width of each
line.

I choose to explictly set `max_width` to its default value of 100 for
clarity.

The reason why we don't see any other changes to the diff should be
obvious: setting `comment_width` to more than `max_width` was useless!

Inspired by:
rust-lang/rustfmt#3349 (comment).
bors bot added a commit to get10101/itchysats that referenced this issue Jul 28, 2022
2572: Set `rustfmt` max comment and line width to 100 r=luckysori a=luckysori

It was a bit weird that the `comment_width` was configured to be _longer_ than the `max_width` which sets the maximum width of each line.

I choose to explicitly set `max_width` to its default value of 100 for clarity.

The reason why we don't see any other changes to the diff should be obvious: setting `comment_width` to more than `max_width` was useless!

Inspired by: rust-lang/rustfmt#3349 (comment).

Co-authored-by: Lucas Soriano del Pino <[email protected]>
@calebcartwright
Copy link
Member

will add explicitly, and more verbosely, that I'd prefer to see the default for this option be the same value as max_width as well. AFACIT the default value of 80 for this was established way back in 2015 in a rather ad-hoc manner during the earliest days of rustfmt development, and it seems to have continued on being the default ever since without any explicit discussion and reasoning around why it is less than max_width (at least none that I could find, though happy to be proven wrong).

@tgross35
Copy link
Contributor

tgross35 commented Apr 1, 2023

Would it be possible to default to whatever max_width is, instead of having a fixed default value?

I'm thinking that in most cases, anybody who changes max_width would probably want comment_width to change with it. Then explicitly setting comment_width would override that if needed

@c-git
Copy link

c-git commented Sep 3, 2023

I remember looking up what the problem with stabilizing this was. But I can't remember where to find it. Can someone point me in the right direction please.

Since I'm commenting I'll just add a +1 for the single setting to max_width possibly with override as mentioned by tgross35 (Unless there is some reason why this is more complicated than it would seem).

@calebcartwright
Copy link
Member

Would it be possible to default to whatever max_width is, instead of having a fixed default value?

Yes, there's a number of width related options implemented this way

I remember looking up what the problem with stabilizing this was. But I can't remember where to find it. Can someone point me in the right direction please.

More generally, see #5365 #5367

In this particular case, there's a salient, unresolved question of what the default value should be.

It's a question on the Style Team's radar as well, but full disclosure, I do not anticipate this is an option that'll be stabilized in the immediate future (i.e. I'd be surprised if this is done by year end, but suspect it will be by the time the 2024 edition drops)

@c-git
Copy link

c-git commented Sep 11, 2023

Thank you appreciate the links. I wasn't sure where to look for them.

@gilescope
Copy link

What's the current default value when I run cargo fmt on stable? (is it to not alter the comment width or to alter it to a fixed width?). Regardless there's already a default in stable (whether we like that default or not is another question); I would argue it shouldn't hold up stabilizing the mechanism for overriding that default.

@calebcartwright
Copy link
Member

calebcartwright commented Apr 9, 2024

What's the current default value when I run cargo fmt on stable?

The current default is 80
https://rust-lang.github.io/rustfmt/?version=v1.6.0&search=#comment_width

However, rustfmt doesn't format comments on stable, so this option isn't operative on stable

The description of the option is:

Maximum length of comments. No effect unless wrap_comments = true.

But wrap_comments itself is also unstable and requires nightly because comment formatting is still unstable
https://rust-lang.github.io/rustfmt/?version=v1.6.0&search=#wrap_comments

I would argue it shouldn't hold up stabilizing the mechanism for overriding that default.

I understand where you're coming from with this argument, but the default value for a config option (regardless of whether the option is a boolean, numeric, etc. type) is a critical factor and part of the long established process for stabilizing options.

Whether there could or should be changes to that process is fair topic for discussion, but so long as that is the process then that's what will be followed for all options, including this one.

(edit: clarified answer as I'd originally missed the "on stable" suffix of the question)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
unstable option tracking issue of an unstable option
Projects
None yet
Development

No branches or pull requests

6 participants