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

Differentiate line and block comments #626

Merged
merged 1 commit into from Feb 7, 2019

Conversation

Projects
None yet
4 participants
@Aerijo
Copy link
Member

Aerijo commented Nov 11, 2018

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • All new code requires tests to ensure against regressions

Description of the Change

Tree-sitter currently treats all comments as comment.block. However, the distinction between block and line can be useful

For example, I'm trying to write a script that auto-comments newlines when created inside a line comment. I don't want it to do this inside a block comment, but it's difficult to tell which it is using the provided scopes. E.g., the desired behaviour is

// some |text

// some
// |text

but in an actual block comment, we would get

/*
some |text
*/

/*
some
// |text
*/

Alternate Designs

The Tree-sitter grammar could differentiate comments.

Benefits

More precise scoping

Possible Drawbacks

Regular expressions can hurt performance. Hopefully, it can optimise this one, as it's only a constant two character check each time. It'd still feel better if there was a rule to match an exact start string (so just //).

Applicable Issues

NA

'comment': 'comment.block'
'comment': [
{
match: "^//",

This comment has been minimized.

@thomasjo

thomasjo Nov 15, 2018

Member

Line comments can appear anywhere on a line; this only handles cases where it's at the beginning of a line.
We need tests that explicitly handles various line comment scenarios.

This comment has been minimized.

@Aerijo

Aerijo Nov 15, 2018

Author Member

@thomasjo The regex here refers to the start of the comment node, which should always be // if it's a simple line comment. It will catch comments at the end of a line

I'll add more tests though

This comment has been minimized.

@thomasjo

thomasjo Nov 15, 2018

Member

Aaaha, thanks for clarifying that :bowtie:

This comment has been minimized.

@Aerijo

Aerijo Nov 15, 2018

Author Member

Actually, I didn't add tests the first time because none exist yet. I guess I'll (try and) make a new set for Tree-sitter.

This comment has been minimized.

@Aerijo

Aerijo Nov 15, 2018

Author Member

Emphasis on try. There's no point in testing the tree; that's done by the parser's package. And I'm having trouble getting the scopes.

This comment has been minimized.

@maxbrunsfeld

maxbrunsfeld Nov 15, 2018

Contributor

I'm ok with not having tests on these scope mappings. The actual parsing is tested very exhaustively, but testing these kinda feels like testing CSS, or testing a config file.

If you have a strong desire to add tests, I'm ok with it, but I'm also cool merging this as-is.

This comment has been minimized.

@Aerijo

Aerijo Nov 15, 2018

Author Member
  • I think tests are a good idea for the Atom specific behaviour, but was unable to get it working
  • This issue can be raised on almost all the repos though, as they all treat comments as blocks
@Aerijo

This comment has been minimized.

Copy link
Member Author

Aerijo commented Nov 24, 2018

OK, I've given up on tests. I still think they're a good idea in the long run, but this change is pretty benign.

@maxbrunsfeld I plan to raise this on all the repos using TS, as there is nothing particularly special about JavaScript.

@daviwil

This comment has been minimized.

Copy link
Member

daviwil commented Feb 7, 2019

Looks good to me, thanks @Aerijo!

Clarification in the comments

@daviwil daviwil merged commit 975351d into atom:master Feb 7, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.