-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Cleanup for previous #2436
Cleanup for previous #2436
Conversation
Current failing mocha tests were failing from before. Will fix them in a different PR. |
core/ast_node.js
Outdated
* @param {Blockly.Input} parentInput Parent input of the connection or field. | ||
* Given an input find the previous editable field or an input with a non null | ||
* connection in the same block. | ||
* @param {!Blockly.Input} location Current location in the ast. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Phrasing: AST nodes are locations in the AST, but I don't think an input is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eh, it's not the node itself, but it does uniquely identify a position in the AST.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me. It might be worth a followup to unify or pull out a common helper for the input and field prev/next methods. They share a lot of code and we might want to tweak aspects of it in the future.
core/ast_node.js
Outdated
* @param {Blockly.Input} parentInput Parent input of the connection or field. | ||
* Given an input find the previous editable field or an input with a non null | ||
* connection in the same block. | ||
* @param {!Blockly.Input} location Current location in the ast. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eh, it's not the node itself, but it does uniquely identify a position in the AST.
The basics
The details
Resolves
Proposed Changes
Reason for Changes
Test Coverage
Tests written in mocha.
Tested on:
Additional Information