-
Notifications
You must be signed in to change notification settings - Fork 893
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
Partition wrap_comments
into normal
or doc
settings
#5943
base: master
Are you sure you want to change the base?
Conversation
@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 @calebcartwright I'd also like your thoughts on this. |
No, I hadn't considered that. I'm personally unsure of the usecase for |
Got it.
Without getting more feedback from users who already use Still brainstorming here. I think we could alternative implement this by making 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 |
9849736
to
0a6c307
Compare
wrap_doc_comments
out of wrap_comments
wrap_comments
into normal
or doc
settings
Updated to use the last structure you mention, that does seem much nicer to me. |
e9b5ca0
to
4c0eba4
Compare
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 |
4c0eba4
to
3454812
Compare
Switched to manual impls of the traits, I've pushed a commit built on top of this with the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
"all" | "true" => Ok(WrapComments::All), | ||
"doc" => Ok(WrapComments::Doc), | ||
"normal" => Ok(WrapComments::Normal), | ||
_ => Err("Bad variant, expected one of: `Off` `All` `Doc` `Normal`"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think It also makes sense to mention true
and false
here?
Configurations.md
Outdated
|
||
/// # 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for applying the feedback so quickly. This should be the last round of nitpicks 😅
Configurations.md
Outdated
struct Foo {} | ||
``` | ||
|
||
#### `"all"`: | ||
Can also be specified as `wrap_comments = false`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Configurations.md
Outdated
struct Foo {} | ||
``` | ||
|
||
Can also be specified as `wrap_comments = true`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Configurations.md
Outdated
@@ -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"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Let's stay consistent with the capitalization.
- **Default value**: `"off"` | |
- **Default value**: `"Off"` |
08c9607
to
0111775
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
What is this PR waiting on? It has been marked as ready to merge for several months at this point. |
As has been mentioned a few times on #3347 it is useful to be able to treat doc-comments and normal comments differently.