Skip to content
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

[JavaScript] Handle type argument in JSDoc documentation #2629

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

rchl
Copy link
Contributor

@rchl rchl commented Nov 18, 2020

Enables parsing of types that can follow some block tags.

The type is scoped with a single scope instead of trying to parse the
type in a "correct" way (how typescript syntax does for types). This is
for one due to the fact that I don't know how to do it "properly" but
even more importantly because typing it properly would make the type
more noisy visually. The only exception is that the dot is scoped as
an accessor so that it triggers completions (especially useful when
using LSP).

@wbond
Copy link
Member

wbond commented Jan 13, 2021

@Thom1729 Wondering if you have any thoughts on this?

@rchl
Copy link
Contributor Author

rchl commented Feb 8, 2021

@Thom1729 ping :)

@Thom1729
Copy link
Collaborator

Thom1729 commented Feb 9, 2021

Whoops; I started typing a response and never posted it.

Because JSDoc is nonstandard, there is some risk to including it in the core syntax. Right now, the implementation is very simple and the chance of conflict is small.

JSDoc type expressions apparently use the syntax from Google's Closure compiler. This syntax looks to be fairly complicated — comparable to TypeScript's type syntax, though not compatible with it. In fact, it looks like TypeScript will read JSDoc type annotations, and it will accept them in either Closure or TypeScript form, which sounds like it would be fun to parse. I think that full support for Closure type expressions would be a lot more complexity than we'd want to add to the core syntax. (There's an open ticket for JS Custom, which I admittedly haven't done anything with.)

Of course, this PR isn't nearly so complicated — it's a minor improvement that doesn't require that much code. It's hard for me to evaluate because I usually try to start from an underlying guiding principle and apply it to the particular case. Here we don't have clear guidelines. If JSDoc were a standard language feature, I'd say we should go the whole way and implement the Closure syntax. But since it's not, I'd rather err on the side of caution.

It might help if I better understood the specific use cases here. Is the main benefit improved LSP completions? Are there any simple examples of annotations which look bad with the current highlighting?

@rchl
Copy link
Contributor Author

rchl commented Feb 9, 2021

Because JSDoc is nonstandard, there is some risk to including it in the core syntax. Right now, the implementation is very simple and the chance of conflict is small.

Note that the implementation has already started before with #2557. That initial PR implemented parsing of @tags. This extends the support further into types. And potentially there is gonna be a follow-up with implementation of @param'eter names (foo in @param {string} foo This is a parameter).

JSDoc type expressions apparently use the syntax from Google's Closure compiler. ... I think that full support for Closure type expressions would be a lot more complexity than we'd want to add to the core syntax. (There's an open ticket for JS Custom, which I admittedly haven't done anything with.)

Yes, not only it would be very complicated to implement full parsing of types but also we really wouldn't want to do that because it would make the JSDoc comments really noisy without providing any additional benefit.

It might help if I better understood the specific use cases here. Is the main benefit improved LSP completions?

The main reason for implementing parsing of JSDoc comments was IMO to make them stand-out and improve readability. It's easier to parse them visually when there is some mild highlighting of special keywords and structure is easy to see at a glance rather than just having a gray block of documentation text.

That said, we don't want them to stand out too much as then they would look too much like the actual code, blurring the separation of concerns.

So really the highlighting of types implemented in this PR is to make them stand out by applying a single scope (color). Nothing more, nothing less. The special handling of interpunction is a little extra to trigger completions and improve the experience with LSP (without LSP possibly too). It was never the intention (and IMO shouldn't be) to parse them in the same way as we do for TS types (or worse, implementing parsing of Closure types whose popularity is pale comparing to TS).

Are there any simple examples of annotations which look bad with the current highlighting?

Well, without this PR JSDoc types look like a plain comment so I wouldn't call it "bad" but it's lacking the semantic highlighting. :)

@rchl
Copy link
Contributor Author

rchl commented Mar 16, 2021

Can we decide on something here?

I'm running with those changes since I've made them and I can't complain about anything. Nothing annoys me about it and documentation blocks are more readable IMO.

Since the highlighting of tags was already implemented and merged, I think it would be weird to stop now. At least if the argument is that this is not a standard language feature.

Also, various other language syntaxes (PHP for example) also have support for highlighting the documentation comments and those are not always an integral part of those languages either.

@Thom1729
Copy link
Collaborator

This should be fine to merge. Even if parsing JSDoc seems messy to me in principle, this PR itself doesn't make it any messier.

Hopefully one of these days I'll get around to complete JSDoc support in JS Custom, and that might help to illuminate the issue, at least from my own perspective.

@rchl
Copy link
Contributor Author

rchl commented Mar 24, 2021

Pushed a small refactor to avoid scoping leading * when jsdoc types span multiple lines.

Before After
Screenshot 2021-03-24 at 21 19 41 Screenshot 2021-03-24 at 21 19 50

This unfortunately required a bit of ugly solution with negative lookahead that excludes all cases that can be matched and consumed within the type expression.
Couldn't use meta_scope anymore as then I can't remove it when leading * is matched.
Couldn't just do a simple . match as then ligatures wouldn't work within type expressions.

@Thom1729
Copy link
Collaborator

Thom1729 commented Apr 6, 2021

Would it work to handle the leading asterisk with a with_prototype rule?

Also, mostly unrelated to this PR, but would it make sense to move the JSDoc implementation out into its own file, like JS regular expressions?

@michaelblyons
Copy link
Collaborator

Also, mostly unrelated to this PR, but would it make sense to move the JSDoc implementation out into its own file, like JS regular expressions?

I believe this is also the way JavaDoc is set up, both currently and in DeathAxe's huge rewrite (#2654).

@rchl
Copy link
Contributor Author

rchl commented Apr 6, 2021

Hmm, not sure how I could use with_prototype here that would help with the "hacky" part.

Basically what would help is clearing the currently set meta_scope just for a single match but I don't think that's possible.

@rchl
Copy link
Contributor Author

rchl commented Apr 6, 2021

I can extract the code into a separate file once/if this gets merged so that the changes done here are easy to diff.

@rchl
Copy link
Contributor Author

rchl commented Oct 1, 2021

Force-pushed since I inadvertently did a rebase locally instead of merging.

Still not clear if there is even a will to get it in but will keep it open until some concrete decision is made.

I will just say that this is something that is provided by VSCode so not having it can be seen as a disadvantage to some.

@rchl
Copy link
Contributor Author

rchl commented Nov 10, 2021

Since there doesn't seem to be a consensus whether this should be merged (not sure why), maybe you would accept this in JSCustom as an option @Thom1729 ?

Enables parsing of types that can follow some block tags.

The type is scoped with a single scope instead of trying to parse the
type in a "correct" way (how typescript syntax does for types). This is
for one due to the fact that I don't know how to do it "properly" but
even more importantly because typing it properly would make the type
more noisy visually. The only exception is that the dot is scoped as
an accessor so that it triggers completions (especially useful when
using LSP).
@rchl
Copy link
Contributor Author

rchl commented May 13, 2023

Not that I have hope that this will get merged but since I'm using it myself, I'll keep pushing my changes here until someone closes it :)

I've changed the scope for type annotations from support.type to meta.type to match the other Python PR.

With meta.type there is no coloring (in Mariana at least) so maybe that's less controversial. It still allows the user to apply own styling with a custom color scheme rule. And there is still added benefit of scoping dot as punctuation to allow for completions to trigger.

JavaScript/JavaScript.sublime-syntax Outdated Show resolved Hide resolved
JavaScript/JavaScript.sublime-syntax Show resolved Hide resolved
JavaScript/JavaScript.sublime-syntax Outdated Show resolved Hide resolved
JavaScript/JavaScript.sublime-syntax Outdated Show resolved Hide resolved
JavaScript/JavaScript.sublime-syntax Outdated Show resolved Hide resolved
Co-authored-by: Keith Hall <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants