-
Notifications
You must be signed in to change notification settings - Fork 15
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
Add support for keeping single line comments from typescript source code #64
base: main
Are you sure you want to change the base?
Conversation
|
||
print(/* this is block comment */"l") | ||
|
||
print( |
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.
It's extremely cool that we can do inline comments without pulling them out into the previous statement... but does it work in the even more general case? I mean, TS can handle all sorts of weird stuff, like:
let foo = { bar: 1 }
console.log(foo. /* Hello */ bar)
That's actually valid and it does indeed print 1!
I feel like Godot would choke on that if we did it this way! Or would it?? Why don't we toss a few of these "weirder" cases into our test suite and test the codegen in Godot? If they all pass, that would be awesome, but if not, we could pull out inline comments into the previous line, e.g. the codegen for the above would be
var foo = { "bar" : 1 }
# Hello
console.log(foo.bar)
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.
If you do decide to go by the per-line route, I think the proper place to inject the comments would be here:
https://github.com/johnfn/ts2gd/blob/main/parse_node.ts#L195
Basically all code in ts2gd flows through combine
so this is a pretty safe place to put it. We have this notion of beforeLines
and afterLines
that allow you to say "emit this code as a standalone line before/after the line currently being codegened" so we could use that for comments as well.
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.
It's extremely cool that we can do inline comments without pulling them out into the previous statement... but does it work in the even more general case? I mean, TS can handle all sorts of weird stuff, like:
I'm a bit confused because this example print(/* this is block comment */"l")
shows that all block/multiline /* ... */
comments are currently strpped out. Did I misunderstand something?
I didn't implement block comments because I do not really know how to handle them and I think they can be separated into another PR. In this PR I wanted to create base algorithm for comment parsing and only focus on single line comments :)
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.
Oh - good point on the block comments, I glossed over that. However, what about something like this?
let foo = { bar: 1 }
console.log(foo.
// Hello
bar)
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.
I realize that specifically may seem like an obscure case, but I think the thing that I'm more generally worried about is that there are possibly a lot of cases like this: you can really put comments almost anywhere in JS/TS, but if you map that 1:1 to Godot, there might be a lot of weird cases that don't compile correctly.
|
||
leadingComments = leadingComments.filter( | ||
(v) => | ||
!props.commentsStack!.find((c) => c.pos === v.pos && c.end === v.end) |
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.
I believe you that we need to dedupe them, but why? Does TS give us back the same comment multiple times? When and why?
It might help to leave a comment about this.
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.
The problem is that multiple nodes can give the same comment when they are in the same line. For example:
// some comment
myFunc(1 + 2, someVar)
This code generates AST like: CallExpression (params: BinaryExpression (left: NumberLiteral (1), right: NumberLiteral (2)), Identifier (someVar))
. All nodes in this AST will report // some comment
that is why we need to handle only first node (CallExpression) that report this comments and ignore the rest otherwise we would generate # some comment
in .gd
four times. I will add some comments.
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.
Ah, I understand. Bit of a wild guess here, but maybe comments are mapped 1:1 with statements, and then copied into child nodes? Could you try just checking if node is a statement type and ONLY if it is appending the comments?
e.g. like this:
https://github.com/johnfn/ts2gd/blob/main/parse_node.ts#L169
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.
Because it seems if we could solve this then a lot of this code would be simplified - we wouldn't even need to track comments at all in the ParseState!
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.
I tried limiting it only to statements but then cases like one below are removed. I don't think comments are mapped 1:1 with statements because when single statement is divided to multiple lines and there are comments inside those lines then comments are bound to expression nodes and not to the statement.
someCall(
// parameter comment
"abcd",
// next comment
123
)
someCall("abcd", 123)
I also tested your example from other comment and indeed it generates invalid GDScript code, so I think we somehow handle these weird cases or we do as you said: limit comment generation only for statements and this way we won't need too worry about such cases.
// example from other comment
let foo = { bar: 1 }
console.log(foo.
// Hello
bar)
I don't really know which way to move forward so I'll wait for your opinion. Also there is not so good another way, just leave it as it is and if someone creates comment that generates invalid GDScript then Godot will actually tell him that something is wrong anyway.
@@ -95,6 +98,7 @@ export class Paths { | |||
"dynamic" | |||
) | |||
|
|||
this.removeComments = tsgdJson.removeComments || false |
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.
nit: ?? (maybe change the other line too :P)
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.
You mean why this is in Paths
class or why this is called like that?
It is in Paths
because I couldn't find another class which have access to ts2gd.json
file and I wanted comment generation to be configurable from config.
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.
Oh I just mean instead of || we can use ?? e.g. this.removeComments = tsgdJson.removeComments ?? false
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.
Right, still getting used to newer TypeScript after working a lot in old JS code :P
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.
:D I know that feeling. Welcome to the future my friend!!
This is really awesome, way to go! Couple of medium sized issues but nothing major. I think the strategy may end up being to push this into |
I'm actually OK with preserving only statement-level comments, and just disregarding all inline comments for now. Once you fix that up let me know and we should be good to go! |
Add support for generating single line comments in gdscript from original typescript code. Multiline comments are still not supported and will be stripped out. The order and generation of comments depends on code in
parse_node/
directory. If some comments are out of place or not showing up then we will have to adjust parsing in the future.I think it closes #29 for now.
Example code:
Generates: