Partition `wrap_comments` into `normal` or `doc` settings #5943

Nemo157 · 2023-10-22T01:28:39Z

As has been mentioned a few times on #3347 it is useful to be able to treat doc-comments and normal comments differently.

ytmimi · 2023-10-22T17:37:01Z

@Nemo157 thanks for taking the time to put together a PR. I haven't had a chance to review this yet, but I noticed that a lot of tests needed the addition of wrap_doc_comments. I'm wondering if you considered allowing wrap_comments=true to imply wrap_doc_comments=true if wrap_doc_comments isn't explicitly set. That way the addition of wrap_doc_comments wouldn't impact users who're already used to the current wrap_comments behavior. We can technically change the behavior of unstable options like wrap_comments, but I think it would be better if we didn't.

@calebcartwright I'd also like your thoughts on this.

Nemo157 · 2023-10-22T17:49:01Z

No, I hadn't considered that. I'm personally unsure of the usecase for wrap_comments itself, I've only ever thought the doc-comment variant was useful, so I'm not sure if that implication would be useful or annoying to people that have a usecase for it.

ytmimi · 2023-10-22T18:28:41Z

No, I hadn't considered that.

Got it.

I'm personally unsure of the usecase for wrap_comments itself, I've only ever thought the doc-comment variant was useful, so I'm not sure if that implication would be useful or annoying to people that have a usecase for it.

Without getting more feedback from users who already use wrap_comments=true to wrap both doc comments and regular comments I think the safest option would be to maintain the current behavior.

Still brainstorming here. I think we could alternative implement this by making wrap_comments an enum instead of a boolean option. Something like:

enum WrapComments {
    /// Don't wrap comments
    Off,
    /// Wrap all kinds of comments
    All,
    /// Only wrap doc comments
    Doc,
    /// Only wrap normal comments
    Normal
}

Then we could map true => All, and false => Off when parsing command line arguments.

Nemo157 · 2024-02-05T11:47:00Z

Updated to use the last structure you mention, that does seem much nicer to me.

ytmimi · 2024-02-06T16:43:48Z

That's great! Thanks for making the change. I took a really quick look and I'm wondering if it would be possible to implement this without any proc macro changes? I know there might be a little more boilerplate, but I think it would be easier for me to review if the necessary traits were manually implemented. I hope that's not too big of an ask.

That being said, I also think the #[bool = true] and #[bool = false] attribute you're proposing could be useful in the future and I'd like to explore that idea in a follow up PR. If you're interested, could you open an issue where we could discuss what those proc macro changes might look like.

Nemo157 · 2024-02-06T21:02:03Z

Switched to manual impls of the traits, I've pushed a commit built on top of this with the #[bool] attribute in the proc-macro to Nemo157@d097616

ytmimi

Thank you very much for taking the time to manually implement those traits. I definitely feel like we're on the right track here!

src/config/options.rs

ytmimi · 2024-02-08T00:21:31Z

src/config/options.rs

+            "all" | "true" => Ok(WrapComments::All),
+            "doc" => Ok(WrapComments::Doc),
+            "normal" => Ok(WrapComments::Normal),
+            _ => Err("Bad variant, expected one of: `Off` `All` `Doc` `Normal`"),


Do you think It also makes sense to mention true and false here?

Configurations.md

src/config/options.rs

tests/source/configs/wrap_comments/all.rs

ytmimi · 2024-02-08T00:54:13Z

Configurations.md

-
-/// # This doc comment is a very long header (it starts with a '#'). Had it not been a header it would have been wrapped. But because it is a header, it will not be. That is because wrapping a markdown header breaks it.


I'm fine with removing this from the example code, but I'd like to preserve this information by adding it to the note above. I'm thinking something like this:

Break comments to fit on the line Note that no wrapping will happen if: 1. The comment is the start of a markdown header doc comment. For example: ```rust /// # This doc comment is a markdown header ``` 3. An URL was found in the comment

ytmimi

Thanks for applying the feedback so quickly. This should be the last round of nitpicks 😅

ytmimi · 2024-02-09T19:52:31Z

Configurations.md

 struct Foo {}
 ```

-#### `"all"`:
+Can also be specified as `wrap_comments = false`.


Is it possible to move this note above the code block? Or would doing so cause issues when we test the formatting? I feel like that will make it easier for users to see, but fine with leaving this here if it causes issues when testing.

ytmimi · 2024-02-09T19:53:11Z

Configurations.md

+struct Foo {}
+```
+
+Can also be specified as `wrap_comments = true`.


Same as above. I think it'll be easier to spot if this note comes before the example code block. Especially now that we've extended it with more examples.

ytmimi · 2024-02-09T19:56:26Z

Configurations.md

@@ -3096,36 +3096,88 @@ Note that no wrapping will happen if:
 1. The comment is the start of a markdown header doc comment
 2. An URL was found in the comment

- **Default value**: `false`
- **Possible values**: `true`, `false`
+- **Default value**: `"off"`


nit: Let's stay consistent with the capitalization.

Suggested change

- **Default value**: `"off"`

- **Default value**: `"Off"`

ytmimi

@Nemo157 just took another look and I think we're good to go. Thanks again for working on this one! I'd like to give @calebcartwright a chance to review these changes, but I'll mark this as ready-to-merge.

VorpalBlade · 2024-07-28T07:23:15Z

What is this PR waiting on? It has been marked as ready to merge for several months at this point.

rustbot added the pr-not-reviewed label Oct 22, 2023

ytmimi added pr-waiting-on-author and removed pr-not-reviewed labels Nov 26, 2023

Nemo157 force-pushed the wrap-doc-comments branch from 9849736 to 0a6c307 Compare February 5, 2024 11:46

Nemo157 changed the title ~~Split wrap_doc_comments out of wrap_comments~~ Partition wrap_comments into normal or doc settings Feb 5, 2024

Nemo157 force-pushed the wrap-doc-comments branch 2 times, most recently from e9b5ca0 to 4c0eba4 Compare February 5, 2024 11:52

Partition wrap_comments into normal or doc settings

3454812

Nemo157 force-pushed the wrap-doc-comments branch from 4c0eba4 to 3454812 Compare February 6, 2024 20:58

ytmimi added pr-follow-up-review-pending and removed pr-waiting-on-author labels Feb 7, 2024

ytmimi reviewed Feb 8, 2024

View reviewed changes

ytmimi added pr-waiting-on-author and removed pr-follow-up-review-pending labels Feb 8, 2024

Move WrapComments into own module, include aliases in all errors

21719e2

ytmimi reviewed Feb 9, 2024

View reviewed changes

Nemo157 added 2 commits February 11, 2024 11:30

Include examples of all wrap_comments settings and header handling

d64fb77

Capitalize wrap-comments settings in tests

0111775

Nemo157 force-pushed the wrap-doc-comments branch from 08c9607 to 0111775 Compare February 11, 2024 10:30

ytmimi approved these changes Feb 13, 2024

View reviewed changes

ytmimi added pr-ready-to-merge release-notes Needs an associated changelog entry and removed pr-waiting-on-author labels Feb 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Partition `wrap_comments` into `normal` or `doc` settings #5943

Partition `wrap_comments` into `normal` or `doc` settings #5943

Nemo157 commented Oct 22, 2023

ytmimi commented Oct 22, 2023

Nemo157 commented Oct 22, 2023

ytmimi commented Oct 22, 2023

Nemo157 commented Feb 5, 2024

ytmimi commented Feb 6, 2024

Nemo157 commented Feb 6, 2024

ytmimi left a comment

ytmimi Feb 8, 2024

ytmimi Feb 8, 2024

ytmimi left a comment

ytmimi Feb 9, 2024

ytmimi Feb 9, 2024

ytmimi Feb 9, 2024

ytmimi left a comment

VorpalBlade commented Jul 28, 2024


		/// # This doc comment is a very long header (it starts with a '#'). Had it not been a header it would have been wrapped. But because it is a header, it will not be. That is because wrapping a markdown header breaks it.

Partition wrap_comments into normal or doc settings #5943

Are you sure you want to change the base?

Partition wrap_comments into normal or doc settings #5943

Conversation

Nemo157 commented Oct 22, 2023

ytmimi commented Oct 22, 2023

Nemo157 commented Oct 22, 2023

ytmimi commented Oct 22, 2023

Nemo157 commented Feb 5, 2024

ytmimi commented Feb 6, 2024

Nemo157 commented Feb 6, 2024

ytmimi left a comment

Choose a reason for hiding this comment

ytmimi Feb 8, 2024

Choose a reason for hiding this comment

ytmimi Feb 8, 2024

Choose a reason for hiding this comment

ytmimi left a comment

Choose a reason for hiding this comment

ytmimi Feb 9, 2024

Choose a reason for hiding this comment

ytmimi Feb 9, 2024

Choose a reason for hiding this comment

ytmimi Feb 9, 2024

Choose a reason for hiding this comment

ytmimi left a comment

Choose a reason for hiding this comment

VorpalBlade commented Jul 28, 2024

Partition `wrap_comments` into `normal` or `doc` settings #5943

Partition `wrap_comments` into `normal` or `doc` settings #5943