-
-
Notifications
You must be signed in to change notification settings - Fork 112
Doc comments #2 #128
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
Doc comments #2 #128
Conversation
// Might have already processed a doc comment in the loop above | ||
if (lexer->result_symbol != DOC_COMMENT) { | ||
lexer->result_symbol = LINE_COMMENT; | ||
while (lexer->lookahead != '\n' && lexer->lookahead != 0) { |
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.
Maybe worth mentioning that Rust requires lines to end with \n
according to https://doc.rust-lang.org/reference/tokens.html#string-literals, thus \r
doesn't need to be checked for those loops.
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.
That seems to be only for string literals though? I've certainly had .rs
files with CRLF endings before
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.
https://doc.rust-lang.org/reference/comments.html#doc-comments
Isolated CRs (\r), i.e. not followed by LF (\n), are not allowed in doc comments.
So it will never be standalone, but can be a part of a line ending.
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've certainly had .rs files with CRLF endings before
What I am trying to imply by that explanation is that even in the case of CRLF the line will still end with \r\n
, not \r
alone, therefore checking only for \n
should detect all line endings.
72001be
to
4762c74
Compare
4762c74
to
61190d9
Compare
@resolritter any updates on this? This (and also code highlighting in docstring examples) are places where |
I'm having a look at this in the hope that we can inject a markdown parser in doc comments. This is obviously a great start, but from what I can tell it's not quite optimal for that scenario. I think we want:
That way, when you pass the contents of doc_comment to a injected markdown parser, it doesn't have all the comment symbol noise and mess up the markdown parser. It would be much nicer if the lexer could skip the start of each line using |
Bumping, as it would be great to finally get some more activity on this. How would this idea interact with code examples? It'd be great if writing examples could be as seemless as writing normal rust code, where each subsequent line gets a |
@mkrasnitski Maybe you could pick up the PR and finish the implementation? @cormacrelf We've solved this for other grammars by using combined injections, @the-mikedavis explains it here helix-editor/helix#4870 (comment) |
I'm just a casual observer in this case - I don't yet feel qualified enough to pick this up myself, as I have really no familiarity with tree sitter internals. This is just one place where I feel tree sitter is not at feature parity with the official |
The combined injections trick works. Thanks for the tip @archseer! Will PR separately, based on this PR (to handle the more-than-three-/ case, which Erlang doesn't have to worry about). The only thing I can't do is get Neovim to use Rust highlights for fenced code blocks without an explicit language tag. Tangentially I think you could do that by adding a feature to nvim-treesitter where you can create an alias for the markdown parser, but with separate query files (that will most likely use |
I will not go forward with this PR. Feel to continue the work in a new PR. |
I suspect the main problem with #99 is what is pointed out in #99 (comment). Here I tried to fix that problem and also add some recursion guard tests just in case. EDIT: Some other cases were not being handled properly in #99 so I've included them and also added more tests.
This is being submitted as Draft so that @maxbrunsfeld can run the "randomized tests". I don't know how to run them.
#99 (comment) might be relevant and invalidate this approach altogether.