Skip to content

[Merged by Bors] - Adds rustfmt configs to wrap and limit comment width#1603

Closed
mnmaita wants to merge 2 commits intobevyengine:mainfrom
mnmaita:feat/add-comment-formatting-config
Closed

[Merged by Bors] - Adds rustfmt configs to wrap and limit comment width#1603
mnmaita wants to merge 2 commits intobevyengine:mainfrom
mnmaita:feat/add-comment-formatting-config

Conversation

@mnmaita
Copy link
Member

@mnmaita mnmaita commented Mar 9, 2021

Aims to close #1594.

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

wrap_comments: rust-lang/rustfmt#3347

comment_width: rust-lang/rustfmt#3349

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 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.

@alice-i-cecile
Copy link
Member

This should work, especially as the situation around #1309 improves :) Yes please to normalize_comments as well; the aim is to attempt to reduce pointless friction on PRs wherever possible.

@alice-i-cecile alice-i-cecile added A-Build-System Related to build systems or continuous integration C-Docs An addition or correction to our documentation labels Mar 9, 2021
Check rust-lang/rustfmt#3350 to track
stabilization of this option.
@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Mar 9, 2021
@cart
Copy link
Member

cart commented Mar 10, 2021

The output looks good to me. Im sold

@cart
Copy link
Member

cart commented Mar 10, 2021

bors r+

bors bot pushed a commit that referenced this pull request 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.
@bors
Copy link
Contributor

bors bot commented Mar 10, 2021

@bors bors bot changed the title Adds rustfmt configs to wrap and limit comment width [Merged by Bors] - Adds rustfmt configs to wrap and limit comment width Mar 10, 2021
@bors bors bot closed this Mar 10, 2021
@Ratysz
Copy link
Contributor

Ratysz commented Mar 14, 2021

I think this might not be accounting for indentation? See #1647, the doc on new methods has a line that's just long enough to fit without indentation, and CI passes.

@cart
Copy link
Member

cart commented Mar 14, 2021

CI doesn't run the new checks at all (note that they are "unstable" features and are commented out).

@cart
Copy link
Member

cart commented Mar 14, 2021

I'm planning on using them like I currently use the "merge imports" setting. Its "nice to have" and I'll run it periodically, but its not enforced.

@Ratysz
Copy link
Contributor

Ratysz commented Mar 14, 2021

... oh, I completely missed that. I guess I was confused by the relevant issue getting closed.

@cart
Copy link
Member

cart commented Mar 14, 2021

Ah yeah we should probably leave it open until it stabilizes (or we move to nightly formatting).

@mnmaita mnmaita deleted the feat/add-comment-formatting-config branch March 30, 2024 13:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-Build-System Related to build systems or continuous integration C-Docs An addition or correction to our documentation S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add CI task to automatically format docstring comment line lengths

4 participants