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 a command to log the syntax tree #18254

Merged
merged 4 commits into from Oct 18, 2018

Conversation

Projects
None yet
2 participants
@Ben3eeE
Member

Ben3eeE commented Oct 17, 2018

Requirements for Adding, Changing, or Removing a Feature

  • Fill out the template below. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • The pull request must contribute a change that has been endorsed by the maintainer team. See details in the template below.
  • The pull request must update the test suite to exercise the updated functionality. For guidance, please see https://flight-manual.atom.io/hacking-atom/sections/writing-specs/.
  • After you create the pull request, all status checks must be pass before a maintainer reviews your contribution. For more details, please see https://github.com/atom/atom/tree/master/CONTRIBUTING.md#pull-requests.

Description of the Change

This adds back the functionality that the editor:log-cursor-scope command used to provide when tree-sitter was used. The command will run default back to editor:log-cursor-scope if text mate is used for the current editor.

This is because the command is very useful when developing grammars 馃檪

@maxbrunsfeld Opening this as a PR to

  • See if you are ok with adding back this functionality with a new command name
  • Ask what we should do when it's run on editors with textmate language mode
  • Ask if this implementation looks ok
  • Ask if I'm naming things in a way that makes sense 馃槄

If all is 馃憤 I can write some tests for this functionality and fill out the rest of the template to get this PR merged.

Verification Process

  • Run the command editor:log-cursor-syntax-tree-scope in a JavaScript file with tree-sitter enabled
    • Expected: Show a notification with the syntax tree scope descriptor
  • Run the command editor:log-cursor-syntax-tree-scope in a JavaScript file with tree-sitter disabled
    • Expected: Show a notification with the scope descriptor
    • Expected: Same ScopeDescriptor is in a notification when also running the editor:log-cursor-scope command
  • Run the command editor:log-cursor-syntax-tree-scope in a CoffeScript file with tree-sitter enabled
    • Expected: Show a notification with the scope descriptor
    • Expected: Same ScopeDescriptor is in a notification when also running the editor:log-cursor-scope command
  • Run the command editor:log-cursor-syntax-tree-scope in a CoffeScript file with tree-sitter disabled
    • Expected: Show a notification with the scope descriptor
    • Expected: Same ScopeDescriptor is in a notification when also running the editor:log-cursor-scope command
  • Bind the command in your keymap and check that it works when running it from a keybinding
  • Passing tests

CoffeScript is a language that currently does not have tree-sitter syntax highlighting

Keymap used for testing the command from a keymap:

'atom-text-editor:not([mini])':
  'ctrl-shift-g': 'editor:log-cursor-syntax-tree-scope'
@maxbrunsfeld

maxbrunsfeld requested changes Oct 17, 2018 edited

I left a few comments. I'm not 100% sure on the naming so I proposed a few options.

// Public: Retrieves the syntax tree descriptor for the cursor's current position.
//
// Returns a {ScopeDescriptor}
getSyntaxTree () {

This comment has been minimized.

@maxbrunsfeld

maxbrunsfeld Oct 17, 2018

Contributor

Regarding naming - I think getSyntaxTree makes it sound like you are retrieving the entire tree. Since we're returning a scope descriptor, which describes a path from the root of the tree to the current node, how about something like:

  • getSyntaxTreePath
  • getSyntaxTreePathDescriptor
  • getSyntaxTreeScopeDescriptor

Thoughts?

This comment has been minimized.

@Ben3eeE

Ben3eeE Oct 18, 2018

Member

getSyntaxTreeScopeDescriptor sounds the best to me considering the other function is getScopeDescriptor and we return a ScopeDescriptor. I will use this one!

@@ -273,6 +273,7 @@ module.exports = ({commandRegistry, commandInstaller, config, notificationManage
@foldAllAtIndentLevel(8)
@scrollToCursorPosition()
'editor:log-cursor-scope': -> showCursorScope(@getCursorScope(), notificationManager)
'editor:log-cursor-syntax-tree': -> showSyntaxTree(@getSyntaxTree(), notificationManager)

This comment has been minimized.

@maxbrunsfeld

maxbrunsfeld Oct 17, 2018

Contributor

If we rename getSyntaxTree, we should rename the command as well.

// * `bufferPosition` A {Point} or {Array} of `[row, column]`.
//
// Returns a {ScopeDescriptor}.
syntaxTreeForBufferPosition (bufferPosition) {

This comment has been minimized.

@maxbrunsfeld

maxbrunsfeld Oct 17, 2018

Contributor

We should adjust the name of this one too.

@@ -3881,6 +3903,11 @@ class TextEditor {
return this.getLastCursor().getScopeDescriptor()
}
// Get the syntax nodes at the cursor.
getSyntaxTree () {

This comment has been minimized.

@maxbrunsfeld

maxbrunsfeld Oct 17, 2018

Contributor

This method name should probably start with getCursor, to make it clear that the positioning is derived from the cursor. Maybe getCursorSyntaxTreePath.

scopes.push(this.grammar.scopeName)
return new ScopeDescriptor({scopes: scopes.reverse()})
}

This comment has been minimized.

@maxbrunsfeld

maxbrunsfeld Oct 17, 2018

Contributor

I think this could be simplified a bit now. We don't necessarily need to exclude anonymous nodes like ( because we're not using the output in getScopeDescriptor.

I think something like this should work:

const nodes = []
this._forEachTreeWithRange(new Range(point, point), tree => {
  let node = tree.rootNode.descendantForPosition(point)
  while (node) {
    nodes.push(node)
    node = node.parent
  }
})

// The nodes are mostly already sorted from smallest to largest,
// but for files with multiple syntax trees (e.g. ERB), each tree's
// nodes are separate. Sort the nodes from largest to smallest.
nodes.reverse()
nodes.sort((a, b) =>
  a.startIndex - b.startIndex || b.endIndex - a.endIndex
)

const nodeTypes = nodes.map(node => node.type)
return new ScopeDescriptor({scopes: nodeTypes})

This comment has been minimized.

@Ben3eeE

Ben3eeE Oct 18, 2018

Member

Used exactly this but I added in the scopeName in the beginning of the ScopeDescriptor and I also added back the point = Point.fromObject(point) line.

This comment has been minimized.

@maxbrunsfeld

maxbrunsfeld Oct 18, 2018

Contributor

Awesome, thanks for taking my partially-working sketch and figuring out how to make it fully correct.

@maxbrunsfeld

Looks great!

@maxbrunsfeld maxbrunsfeld merged commit 11bd210 into master Oct 18, 2018

3 checks passed

Atom Pull Requests #20181018.3 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 b3-log-syntax-tree branch Oct 18, 2018

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