Comment node includes trailing \r

[AndreasArvidsson]

When using CRLF line endings comments will include a trailing \r

# hello
foo: "bar"

node.text: # hello\r

[wenkokke]

This is another scanner bug, unfortunately.

[AndreasArvidsson]

Could you elaborate on that? I'm not quite sure what the scanner means in this context.

This is not the first bug where we've had leading or trailing whitespaces on a node. Would it be worth doing a unit test that checks for leading and/or trailing whitespaces?

[wenkokke]

Scanner means the code that does the lexing; see scanner.cc. It's a bunch of C++ code that implements a custom lexer for TalonScript, and it's where you need to handle any features of the language that are tricky to express as grammars—e.g., indentation sensitivity or lookahead.

[wenkokke]

Could you elaborate on that? I'm not quite sure what the scanner means in this context.

I'd be happy to accept a PR with such tests?

[pokey]

Can you not just tweak the comment regex?

tree-sitter-talon/grammar.js

Line 44 in fd20268

comment: ($) => token(/#.*?/),

[wenkokke]

I'm not sure what purpose that serves, because afaik comment tokens are lexed by the scanner. I guess you could try replacing . by [^\r\n]?

[pokey]

Yeah I was thinking something like that

[pokey]

so is this fixed by #42 ?

[wolfmanstout]

@pokey I'm not sure ... @AndreasArvidsson can you retest this?

I considered adding a unit test for this but it's not easy to capture using the built-in tree-sitter testing system, which doesn't include tests for node contents. I think we'd need to set up a separate unit test, e.g. using the Node.js API -- I'm sure this is easy but I'm just not very familiar with Node.js so it wasn't trivial for me.

[AndreasArvidsson]

@wolfmanstout The problem is still there, but slightly changed. node.text is now "# hello\r\n"

Should definitely be doable with node

[wolfmanstout]

FWIW @wenkokke suggestion above would probably work. Despite the fact that comments are declared as an external they are still parsed by that regex. FWIW this is following the Python implementation pattern. I guess there is some subtle difference, assuming Python doesn't have the same behavior.

[wolfmanstout]

Okay, I have a draft of a fix out:
#45

Before this is merged, I want to point out that the Python tree-sitter grammar has the exact same behavior (I tested it). Should Cursorless just be robust to this instead?

[wenkokke]

Before this is merged, I want to point out that the Python tree-sitter grammar has the exact same behavior (I tested it). Should Cursorless just be robust to this instead?

I based the scanner and grammar on the Python grammar, so they might actually welcome your changes there as well.

Might be less "this behavior is endemic" and more "that's where I copied it from".

That said, probably makes sense for Cursorless to be robust to this.