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

Support a single scope name field on Tree-sitter grammars #17881

Merged
merged 5 commits into from Aug 20, 2018

Conversation

Projects
None yet
2 participants
@maxbrunsfeld
Contributor

maxbrunsfeld commented Aug 20, 2018

Background

When we originally introduced Tree-sitter parsers into Atom, we gave them entirely new root scope names, which we called ids instead of scope names. The plan was to make scope descriptors work differently with the new parsers, containing entries for each node in the syntax tree.

Since then, we've received a lot of issues about the new scope names causing problems. A lot of packages use scope names to control activation. We've also realized that we should be able to simply replace TextMate parsers with Tree-sitter parsers with the same scope name without introducing any breaking changes.

Change

This PR remove the mechanisms for Tree-sitter grammars to track their correspondence to TextMate grammars while keeping separate scope names. Instead, we're just going to use the same scope names. I believe this will result in less overall confusion for users when we turn on Tree-sitter by default.

Fixes #17820
Fixes #17859

Support a single scope name field on Tree-sitter grammars
* Remove the `legacyScopeName` field
* Remove the legacy scope name concept from the Config class
* Handle tree-sitter grammars and textmate grammars having the same 
scope names

maxbrunsfeld added a commit to atom/flight-manual.atom.io that referenced this pull request Aug 20, 2018

@maxbrunsfeld maxbrunsfeld merged commit a0bbc6c into master Aug 20, 2018

3 checks passed

VSTS: Atom Pull Requests 20180820.8 succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@maxbrunsfeld maxbrunsfeld deleted the mb-compatible-scope-descriptors branch Aug 20, 2018

@maxbrunsfeld

This comment has been minimized.

Show comment
Hide comment
@maxbrunsfeld

maxbrunsfeld Aug 21, 2018

Contributor

/cc @Aerijo since will affect your Tree-sitter grammar packages - they need a scopeName instead of an id now. Sorry for the churn and thanks for adopting this experimental API early on.

Contributor

maxbrunsfeld commented Aug 21, 2018

/cc @Aerijo since will affect your Tree-sitter grammar packages - they need a scopeName instead of an id now. Sorry for the churn and thanks for adopting this experimental API early on.

@maxbrunsfeld maxbrunsfeld referenced this pull request Aug 21, 2018

Closed

Turn on tree-sitter by default #17810

2 of 3 tasks complete

@jasonrudolph jasonrudolph referenced this pull request Aug 23, 2018

Closed

Iteration Plan: August 20 - August 31 #17910

13 of 16 tasks complete

@Arcanemagus Arcanemagus referenced this pull request Sep 4, 2018

Open

linter-eslint not showing errors (or even working ?) #1157

3 of 3 tasks complete
@t9md

This comment has been minimized.

Show comment
Hide comment
@t9md

t9md Sep 10, 2018

Contributor

I found another scope incompatibility.

  • editor.bufferRangeForScopeAtPosition() takes scope specifier as string, but behavior is not consistent.

Can I expect this to be fixed too?

When cursor is at | char in following text.

let a, b /* 2nd |var */, c

In both TextMateParser and TreeSitterParser, command Editor: Log Cursor Scope(alt-cmd-p) shows source.js and comment.block.js.
But when I tried to use .comment.block as scope for bufferRangeForScopeAtPosition to retrieve block comment range, TreeSitterParser return nothing whereas TextMateParser return block-comment range.
Seemingly TreeSitter expect different scope string specifier.

const editor = atom.workspace.getActiveTextEditor()
const range = editor.bufferRangeForScopeAtPosition(".comment.block", editor.getCursorBufferPosition())
if (range) {
  console.log(editor.getTextInBufferRange(range))
}
// -> In TextMateParser: output "/* 2nd |var */"
// -> In TreeSitterParser: no ouptut

Some of the vim-mode-plus's function depends on this behavior.

Contributor

t9md commented Sep 10, 2018

I found another scope incompatibility.

  • editor.bufferRangeForScopeAtPosition() takes scope specifier as string, but behavior is not consistent.

Can I expect this to be fixed too?

When cursor is at | char in following text.

let a, b /* 2nd |var */, c

In both TextMateParser and TreeSitterParser, command Editor: Log Cursor Scope(alt-cmd-p) shows source.js and comment.block.js.
But when I tried to use .comment.block as scope for bufferRangeForScopeAtPosition to retrieve block comment range, TreeSitterParser return nothing whereas TextMateParser return block-comment range.
Seemingly TreeSitter expect different scope string specifier.

const editor = atom.workspace.getActiveTextEditor()
const range = editor.bufferRangeForScopeAtPosition(".comment.block", editor.getCursorBufferPosition())
if (range) {
  console.log(editor.getTextInBufferRange(range))
}
// -> In TextMateParser: output "/* 2nd |var */"
// -> In TreeSitterParser: no ouptut

Some of the vim-mode-plus's function depends on this behavior.

@maxbrunsfeld

This comment has been minimized.

Show comment
Hide comment
@maxbrunsfeld

maxbrunsfeld Sep 10, 2018

Contributor

Thanks for pointing that out, @t9md. I will fix this.

Contributor

maxbrunsfeld commented Sep 10, 2018

Thanks for pointing that out, @t9md. I will fix this.

@t9md

This comment has been minimized.

Show comment
Hide comment
@t9md

t9md Sep 11, 2018

Contributor

Thanks for the quick response as always!
I still have several issues on vim-mode-plus when switching from TextMate parser to TreeSitter parser.
Mostly it is related to scopes compatibility and tokenized buffer. Am I OK to ask about it here?

Contributor

t9md commented Sep 11, 2018

Thanks for the quick response as always!
I still have several issues on vim-mode-plus when switching from TextMate parser to TreeSitter parser.
Mostly it is related to scopes compatibility and tokenized buffer. Am I OK to ask about it here?

@t9md

This comment has been minimized.

Show comment
Hide comment
@t9md

t9md Sep 11, 2018

Contributor

@maxbrunsfeld Could you check and give comment on the following issue?
I can't find a good solution for now.
t9md/atom-vim-mode-plus#1095

Contributor

t9md commented Sep 11, 2018

@maxbrunsfeld Could you check and give comment on the following issue?
I can't find a good solution for now.
t9md/atom-vim-mode-plus#1095

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