Convert Tree.java to Tree.ts #5
Conversation
Plus a few hand comments in index.ts
| * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. | ||
| */ | ||
|
|
||
| // ConvertTo-TS run at 2016-10-02T21:58:18.5966470-07:00 |
There was a problem hiding this comment.
💭 We could replace this line with our sign-off comments when we go back and review the file for all the details (completeness, correctness). Then when the initial version of the target is complete we just remove the sign-off comments too.
| * It is the most abstract interface for all the trees used by ANTLR. | ||
| */ | ||
| export interface Tree { | ||
| /** The parent of this node. If the return value is null, then this |
There was a problem hiding this comment.
❓ Would this be a case where we want null or undefined?
There was a problem hiding this comment.
?? You comment doesn't seem to match up with a place that makes sense!
There was a problem hiding this comment.
The comment says the return value can be null.
There was a problem hiding this comment.
OK, the comment. Perhaps, but we're really defining the contract here, rather than testing a value. I see no reason we should define the contract to allow a return value of undefined.
In TypeScript 2 there is a new "strict null checking" option, which requires you to explicitly allow both null and/or undefined in type declarations, or it will flag code which violates this. We should consider if we want to use strict null checking on this project. Opened issue #14 to track.
| * invocation. For abstract syntax trees (ASTs), this is a {@link Token} | ||
| * object. | ||
| */ | ||
| getPayload(): any; |
There was a problem hiding this comment.
💭 In the end, we may not need this method for TypeScript target, but I see no harm in keeping with the "port as close as possible" for now. Maybe we create issues related to potential style changes that could be applied in the future, between when things are first working and when we're ready to 
?
There was a problem hiding this comment.
Yes, I like the idea of tracking possible style changes with issues.
| /** The parent of this node. If the return value is null, then this | ||
| * node is the root of the tree. | ||
| */ | ||
| getParent(): Tree; |
There was a problem hiding this comment.
📝 I filed #8 so we can come back after the initial port and discuss the best pattern to use for this type of read-only property.
|
So what's the protocol, you approve and I merge? |
If I have questions then I'll wait until you answer to merge (in case it triggers some thinking where you want to make a change). Otherwise I'll just merge. If you have a WIP item, add a do not merge label and I won't merge it. |
This is the small example you requested.
I'd like to see how things work if you make a small change in the Java version...