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

detect style prefix in dev comments #133

Open
drahnr opened this issue Jan 4, 2021 · 8 comments · May be fixed by #204
Open

detect style prefix in dev comments #133

drahnr opened this issue Jan 4, 2021 · 8 comments · May be fixed by #204
Assignees
Labels
good first issue 🔰 Good for newcomers help wanted 🤝 Extra attention is needed

Comments

@drahnr
Copy link
Owner

drahnr commented Jan 4, 2021

Is your feature request related to a particular use-case?

The dev comment feature is part of master now.

Currently the asterisk of

/*
 *
 */

in line 2 is part of the reflown components.

Describe the solution you'd like to implement/see implemented

Detecting differently styled /* ... */ comments and stripping leading \s\*\s (regex notation) items and inserting.

Describe alternatives you've considered

Disallow dev comments for reflow operations.

Additional context

Ref #115

@drahnr drahnr added good first issue 🔰 Good for newcomers help wanted 🤝 Extra attention is needed labels Jan 4, 2021
@pjsier
Copy link

pjsier commented Oct 1, 2021

@drahnr I'm interested in taking this on, do you have any thoughts on the best way to implement it?

@drahnr
Copy link
Owner Author

drahnr commented Oct 2, 2021

He @pjsier , that's great! There is already some prefix parsing happening for /// and such.

My suggestion would be to take it from there, and detect if the span contains above pattern, and if so, split it into multiple spans.

I'd be happy to mentor you through this, preferably via a draft PR :)

@pjsier
Copy link

pjsier commented Oct 6, 2021

Thanks! I took a look into the code based on your advice and I'm working on getting something up for a draft PR but running into some issues. The initial parsing in src/documentation/developer.rs seems relatively straightforward since it's splitting everything into separate lines. As a start I'm adding a conditional for assigning pre similar to the way post is assigned in this block:

while let Some(next_line) = lines.next() {
line_number += 1;
let post = if next_line.ends_with(BLOCK_COMMENT_POSTFIX) {
TokenType::BlockComment.post_in_chars()
} else {
0
};
let literal = match TrimmedLiteral::from(
CommentVariant::SlashStar,
next_line,
0,
post,
line_number,
0,
) {

Where I'm running into issues is in src/documentation/literal.rs where it seems like /// is handled. It looks like while literal_set_from_block_comment is splitting tokens on newlines, based on some of the multiline tests here that doesn't seem to be the case in this file:

block_comment_test!(
trimmed_multi_doc,
"/**
mood
*/"
);
block_comment_test!(
trimmed_multi_mod,
"/*!
mood
*/"
);

The main issue I'm running into is that block comment variants support prefixes and always end with the suffix */, but this new prefix would need to be nested inside of that so it doesn't seem like it can be handled the same way as ///.

Do you have any thoughts on the best way to handle that, or am I missing something in the implementation? Thanks again!

@drahnr
Copy link
Owner Author

drahnr commented Oct 6, 2021

Thanks! I took a look into the code based on your advice and I'm working on getting something up for a draft PR but running into some issues. The initial parsing in src/documentation/developer.rs seems relatively straightforward since it's splitting everything into separate lines. As a start I'm adding a conditional for assigning pre similar to the way post is assigned in this block:

Note that there are currently two parsing approaches - one for dev in developer.rs based on ra_ap_syntax and the doc comments in Clusters::parse_token_tree based on syn - both are combined in Clusters::load_from_str.

Mid term only ra_ap_syntax will stay, but this something to keep in mind when testing, since dev comments are not checked by default.

while let Some(next_line) = lines.next() {
line_number += 1;
let post = if next_line.ends_with(BLOCK_COMMENT_POSTFIX) {
TokenType::BlockComment.post_in_chars()
} else {
0
};
let literal = match TrimmedLiteral::from(
CommentVariant::SlashStar,
next_line,
0,
post,
line_number,
0,
) {

Where I'm running into issues is in src/documentation/literal.rs where it seems like /// is handled. It looks like while literal_set_from_block_comment is splitting tokens on newlines, based on some of the multiline tests here that doesn't seem to be the case in this file:

block_comment_test!(
trimmed_multi_doc,
"/**
mood
*/"
);
block_comment_test!(
trimmed_multi_mod,
"/*!
mood
*/"
);

/// is a doc comment, so it originates not in developer.rs, but uses some information as provided by syn. Literal (and by extension TrimmedLiteral) were originally designed to hold the minimal information around that. Syn provides a single multi-line span for /** .. */ and /*! .. */ (they are doc comments too, unlike /* .. */).

The main issue I'm running into is that block comment variants support prefixes and always end with the suffix */, but this new prefix would need to be nested inside of that so it doesn't seem like it can be handled the same way as ///.

This is correct. I think the best approach might be, to split /** .. */ and /*! .. */ into inner line based spans (so the outline span of the comment still exists, but a TrimmedLiteral removes the prefix \s\*\s but retains info about it by adding a new enum field Padding(String) or NoPadding).
An alternative, would be to introduce a "continuation" prefix in CommentVariant::SlashAsterisk{,EM,Asterisk}. This could be checked in LiteralSet::add_adjacent, which already does a check for matching comment types which are in adjacent lines.
These do the same thing, but are two different layers to operate on, with the latter making it harder to refactor in the long run since two layers of abstraction are mushed together.

Do you have any thoughts on the best way to handle that, or am I missing something in the implementation? Thanks again!

I'd suggest to focus either on dev comments or on doc comments first (I recommend doc comments though, it's the harder problem and more people will benefit from that, since dev comment checks are disabled by default). Either way should be fine though :)

Let me know if this makes sense to you or I should elaborate on anything further :)

@pjsier
Copy link

pjsier commented Oct 6, 2021

This makes sense, thanks so much for clarifying!

@pjsier pjsier linked a pull request Oct 10, 2021 that will close this issue
3 tasks
@drahnr
Copy link
Owner Author

drahnr commented Jan 14, 2022

Gentle ping

@pjsier
Copy link

pjsier commented Jan 14, 2022

Sorry for the delay! I don't think I'll be able to wrap this up, so feel free to assign it to someone else, thanks for all of your help!

@drahnr
Copy link
Owner Author

drahnr commented Jan 14, 2022

Alright, thanks for your work so far, I'll take it from here :)

@drahnr drahnr self-assigned this Jan 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue 🔰 Good for newcomers help wanted 🤝 Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants