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

feat: continue single-line comments #8664

Closed

Conversation

seanchen1991
Copy link

@seanchen1991 seanchen1991 commented Oct 30, 2023

Closes: #1730

This is a re-attempt at implementing continue-comment functionality, following the earlier attempt in #1937; I lifted most of @antoyo's logic from his initial PR.

This PR only implements continuation of single-line comments; block comments are left as a TODO. Additionally, I attempted to implement proper indentation in new comment lines as per this comment, but was finding it really finicky to get right, and so I'm opening this PR without proper handling of indentation in continued comments for now. I'll try to open a subsequent PR that adds that logic at a later time.

The functionality is behind a continue-comments configuration option that defaults to enabled. Additionally, a comment-tokens field was added for each language in languages.toml that accepts an array of strings in order to specify which comment tokens to look for. I think there's two ways to handle this: introduce the new comment-tokens field and deprecate the old comment-token field, or implement some logic that parses a string of comma-separated tokens in order to not introduce a breaking change. Which would be the preferred route here?

@seanchen1991 seanchen1991 changed the title Continue single-line comments feat: continue single-line comments Oct 30, 2023
///
/// Return None otherwise.
pub fn get_comment_token<'a>(line: &RopeSlice, tokens: &'a [String]) -> Option<&'a str> {
// TODO: don't continue shebangs
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This TODO was carried over from #1937, but I'm not sure what it means.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

About shebangs in general: https://en.wikipedia.org/wiki/Shebang_(Unix)

We shouldn't continue shebangs as if they were regular comments since they only take up one line

@the-mikedavis
Copy link
Member

#4718 also adds support for multiple tokens. We will most likely merge that PR before we can give this a proper look.

If you're resubmitting work from another PR it's customary to give the original author credit, either by re-using their commits without changes or by giving them co-author on the commits where you re-use their code: https://docs.github.com/en/pull-requests/committing-changes-to-your-project/creating-and-editing-commits/creating-a-commit-with-multiple-authors

@seanchen1991
Copy link
Author

If you're resubmitting work from another PR it's customary to give the original author credit

Is there a way to apply these changes to commits retroactively?

@the-mikedavis
Copy link
Member

See https://docs.github.com/en/pull-requests/committing-changes-to-your-project/creating-and-editing-commits/changing-a-commit-message#amending-older-or-multiple-commit-messages

Rebasing / ammending old commits can be complicated though. We could instead add a co-authored-by tag at merge-time if we squash + merge

@seanchen1991
Copy link
Author

seanchen1991 commented Oct 31, 2023

We could instead add a co-authored-by tag at merge-time if we squash + merge

Sounds good to me 🙂

@bluthej bluthej mentioned this pull request Dec 7, 2023
@TornaxO7
Copy link
Contributor

TornaxO7 commented Apr 8, 2024

@seanchen1991 just a little reminder that the toogle-block-comments PR is merged if you haven't noticed it yet :)

@djugei
Copy link

djugei commented Aug 24, 2024

This change seems to be quite far along, what additional work is necessary to get it merged?

edit: ah i see its continued in #10996

@the-mikedavis
Copy link
Member

Closing in favor of #10996

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

Successfully merging this pull request may close these issues.

Continue comments
4 participants