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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add more scopes to the tree-sitter grammar #303

Merged
merged 5 commits into from Nov 8, 2018

Conversation

Projects
None yet
4 participants
@Ben3eeE
Member

Ben3eeE commented Nov 1, 2018

馃毃 WIP 馃毃

TODO:

Refs atom/atom#18347 (comment)

Depends on: atom/atom#18382

Fixes: #297
Fixes: #296
Fixes: #295
Fixes: #294
Fixes: #293
Fixes: #291

/cc: @maxbrunsfeld

@thomasjo

This comment has been minimized.

Member

thomasjo commented Nov 1, 2018

@Ben3eeE Haven't had time to check this myself yet, but does this cause any false positives with C++ template expressions? E.g. template <typename T>, template <std::vector<T>>, and so forth? For those expressions the brackets should not be marked as operators.

@Ben3eeE

This comment has been minimized.

Member

Ben3eeE commented Nov 1, 2018

@thomasjo Thanks for pointing that out. It's entirely possible that it will conflict with template expressions. I haven't tested this myself yet actually. Just editing through the web editor 馃檪

I will take a look at this as soon as I can. Are there any other examples you can think of?

@thomasjo

This comment has been minimized.

Member

thomasjo commented Nov 1, 2018

@Ben3eeE Can't think of anything else right now, but I'll have a "quick" scan through the language specification later. Will let you know if I find anything exotic 馃榿

@Ben3eeE

This comment has been minimized.

Member

Ben3eeE commented Nov 1, 2018

@thomasjo After atom/atom#18382 is merged this PR as it is now will work and not conflict with template expressions.

TextMate still scopes > and < in template expressions as keyword.operator.comparison so we might want to just leave it scoped incorrectly to not break highlighting of < and > in themes.

@maxbrunsfeld Do you think we should scope > and < as keyword.operator in template expressions to keep the highlighting the same as textmate?

@maxbrunsfeld

This comment has been minimized.

Contributor

maxbrunsfeld commented Nov 1, 2018

Do you think we should scope > and < as keyword.operator in template expressions to keep the highlighting the same as textmate?

I think we shouldn't; that seems like a bug in the TextMate parser. I'd prefer to either leave them as unscoped, or scope them in a way that's similar to the (, ), [, and ] characters.

@thomasjo

This comment has been minimized.

Member

thomasjo commented Nov 1, 2018

I strongly agree with @maxbrunsfeld. That鈥檚 a bug in the TextMate grammar.

@Ben3eeE

This comment has been minimized.

Member

Ben3eeE commented Nov 1, 2018

Seems like we don't have any punctuation scopes in the tree-sitter grammar at all. I will sync it up with textmate and correct this/other bugs 馃憤

Thanks so much for taking a look at this PR @thomasjo it made us notice the bug in atom core and fix up the scopes more 馃挴

@Ben3eeE

This comment has been minimized.

Member

Ben3eeE commented Nov 1, 2018

We have a bunch of opened issues from @chbk regarding punctuation scopes but all are with the textmate grammar. I think it will make sense to fix these for the tree-sitter grammar.

@50Wliu

This comment has been minimized.

Member

50Wliu commented Nov 1, 2018

Another agreement with @maxbrunsfeld. We shouldn't let old bugs with the TextMate grammars affect Tree-sitter.

@Ben3eeE Ben3eeE changed the title from Scope < and > as keyword.operator to Add more scopes to the tree-sitter grammar Nov 2, 2018

@Ben3eeE

This comment has been minimized.

Member

Ben3eeE commented Nov 2, 2018

@maxbrunsfeld I took a look at the open issues we have here to see if I could improve further. Some of these parse incorrectly with tree-sitter. I didn't check if any of these are valid reports. Just blindly assuming that they are.

From a slack chat with @thomasjo:

From open issues:

struct alignas(64) Empty64 {};

alignas(64) is parsed as a type_identifier

Issues we can solve that are also an issue in tree-sitter:
#177 e1362f4

@Ben3eeE Ben3eeE force-pushed the b3-more-operators branch from ec76634 to e1362f4 Nov 3, 2018

@Ben3eeE Ben3eeE referenced this pull request Nov 3, 2018

Closed

Math Operators #19

@maxbrunsfeld

This comment has been minimized.

Contributor

maxbrunsfeld commented Nov 8, 2018

I think let's get this into today's hotfix as-is. We can circle back and fix the parsing issues that you mentioned in another pass.

@maxbrunsfeld maxbrunsfeld merged commit 3571118 into master Nov 8, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@maxbrunsfeld maxbrunsfeld deleted the b3-more-operators branch Nov 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment