Decouple JSDoc from JavaScript grammar / Highlight examples #496

Alhadis · 2017-03-03T16:24:05Z

Fixes #495.

Description of the Change

This PR separates the patterns for highlighting JSDoc tags from the main JavaScript grammar. The lookahead patterns currently responsible for this are inflicting severe performance issues in certain cases, which is to say nothing of the maintainability issues they've always imposed.

Additionally, I've also included inline JS highlighting for @example tags:

Alternate Designs / Thoughts

Initially, I wrote each block tag to span multiple lines, but realised highlighting something like @access \n private really didn't justify the added complexity.

Benefits

Other than being much more readable and maintainable, I imagine this will boost the tokeniser's performance drastically. It might be interesting to benchmark.

Possible Drawbacks

In light of the countless intricacies of Closure Compiler, I've opted to match {types} and @param tags with an inclusive approach, rather than exclusive. Users will no longer be able to rely on what this grammar matches to indicate what syntax is valid. To be honest, I feel it's a fair trade-off... grammars shouldn't be responsible for validating input (at least beyond the scope of unclosed strings, etc).

See https://gist.github.com/infininight/9392d1e4408d507a4691e3daba9b3ec9

This could risk triggering incorrect highlighting if a theme colours the contents of interpolated `${ literal }` sequences.

Alhadis · 2017-03-03T16:24:25Z

/cc @50Wliu @bgriffith

winstliu · 2017-03-06T16:01:23Z

Oh look, another huge PR to review from @Alhadis 😁. I may need to punt this for a while to get a language-c rewrite merged first, as that grammar is in pretty bad shape right now.

winstliu · 2017-03-07T21:56:26Z

grammars/jsdoc.cson

+      '3':
+        'name': 'entity.name.type.instance.jsdoc'
+      '4':
+        'name': 'punctuation.definition.begin.jsdoc'


.bracket.angle

winstliu · 2017-03-07T21:56:41Z

grammars/jsdoc.cson

+    'match': '''(?x)
+      ((@)borrows) \\s+
+      ((?:[^@\\s*/]|\\*[^/])++)   # <that namepath>
+      \\s++ (as) \\s++            # as


Why the possessive match?

winstliu · 2017-03-08T00:56:03Z

grammars/jsdoc.cson

+        # URL
+        (
+          (?=https?://)
+          (?:[^\\s*]|\\*[/])+


Why is the forwards slash in a character class? Did you perhaps forget a ^? I noticed it also looks different from all the other patterns similar to this one.

Good catch, fixed.

winstliu · 2017-03-08T00:56:37Z

grammars/jsdoc.cson

+        # JSDoc namepath
+        (
+          (?!https?://)
+          (?:[^@\\s*/]|\\*[^/])++


Why possessive?

Because I'm a possessive prick.

(removed)*+

winstliu · 2017-03-08T00:57:51Z

grammars/jsdoc.cson

+        [A-Za-z_$]        # First character: non-numeric word character
+        [\\w$.\\[\\]]*    # Rest of identifier
+        (?:               # Possible list of additional identifiers
+          \\s* , \\s*


the comma should be tokenized

winstliu · 2017-03-08T01:08:07Z

grammars/jsdoc.cson

+        ]
+      }
+    ]
+


🔥 empty line

winstliu · 2017-03-08T01:10:58Z

spec/jsdoc-spec.coffee

+    runs ->
+      grammar = atom.grammars.grammarForScopeName("source.js")
+
+


🔥 empty line

winstliu · 2017-03-08T01:12:20Z

spec/jsdoc-spec.coffee

+      expect(tokens[1]).toEqual value: ' Text ', scopes: ['source.js', 'comment.block.documentation.js']
+      expect(tokens[2]).toEqual value: '{@', scopes: ['source.js', 'comment.block.documentation.js', 'entity.name.type.instance.jsdoc', 'storage.type.class.jsdoc', 'punctuation.definition.inline.tag.begin.jsdoc']
+      expect(tokens[3]).toEqual value: 'link', scopes: ['source.js', 'comment.block.documentation.js', 'entity.name.type.instance.jsdoc', 'storage.type.class.jsdoc']
+      expect(tokens[4]).toEqual value: 'plain', scopes: ['source.js', 'comment.block.documentation.js', 'entity.name.type.instance.jsdoc', 'variable.other.description.jsdoc']


This is interesting...shouldn't it be tokenized as linkplain and not separately?

winstliu · 2017-03-08T01:12:29Z

spec/jsdoc-spec.coffee

+      expect(tokens[1]).toEqual value: ' Text ', scopes: ['source.js', 'comment.block.documentation.js']
+      expect(tokens[2]).toEqual value: '{@', scopes: ['source.js', 'comment.block.documentation.js', 'entity.name.type.instance.jsdoc', 'storage.type.class.jsdoc', 'punctuation.definition.inline.tag.begin.jsdoc']
+      expect(tokens[3]).toEqual value: 'link', scopes: ['source.js', 'comment.block.documentation.js', 'entity.name.type.instance.jsdoc', 'storage.type.class.jsdoc']
+      expect(tokens[4]).toEqual value: 'code', scopes: ['source.js', 'comment.block.documentation.js', 'entity.name.type.instance.jsdoc', 'variable.other.description.jsdoc']


winstliu · 2017-03-08T01:17:10Z

spec/jsdoc-spec.coffee

+      expect(tokens[5]).toEqual value: '{', scopes: ['source.js', 'comment.block.documentation.js', 'entity.name.type.instance.jsdoc', 'punctuation.definition.bracket.curly.begin.jsdoc']
+      expect(tokens[6]).toEqual value: '*', scopes: ['source.js', 'comment.block.documentation.js', 'entity.name.type.instance.jsdoc']
+      expect(tokens[7]).toEqual value: '}', scopes: ['source.js', 'comment.block.documentation.js', 'entity.name.type.instance.jsdoc', 'punctuation.definition.bracket.curly.end.jsdoc']
+      expect(tokens[9]).toEqual value: 'variable', scopes: ['source.js', 'comment.block.documentation.js', 'variab


🔥 empty line

winstliu · 2017-03-08T01:18:14Z

Would it be worthwhile to tokenize default values as actual JS?

Alhadis · 2017-03-09T08:45:27Z

Possibly, but that would likely leave us at the mercy of runaway begin/end pairs if the terminating ] was matched by something else.

Alhadis · 2017-03-09T12:41:06Z

... never mind, I said nothing.

I shouldn't be allowed to communicate with humans until I've finished waking up.

winstliu

Note: All I did was highlight instances of possessive matching. I'm unsure if any of those are intentional, if so, just ignore the comment.

winstliu · 2017-03-09T15:11:19Z

grammars/jsdoc.cson

+    'match': '''(?x)
+      ((@)borrows) \\s+
+      ((?:[^@\\s*/]|\\*[^/])+)    # <that namepath>
+      \\s++ (as) \\s+             # as


You forgot to remove the first possessive match on this line.

winstliu · 2017-03-09T15:13:51Z

grammars/jsdoc.cson

+      }
+      {
+        'name': 'entity.name.type.instance.jsdoc'
+        'match': '(?:[^@\\s*/]|\\*[^/])++'


winstliu · 2017-03-09T15:14:50Z

grammars/jsdoc.cson

+      \\s+
+      (
+        [^@\\s<>*/]
+        (?:[^@<>*/]|\\*[^/])*+


winstliu · 2017-03-09T15:16:28Z

grammars/jsdoc.cson

+      }
+      {
+        # Highlighted JavaScript example
+        'match': '[^\\s@*](?:[^*]|\\*[^/])*+'


winstliu · 2017-03-09T15:16:37Z

grammars/jsdoc.cson

+      (?:
+        \\s*
+        (<)
+        ([^>\\s]++)


Alhadis · 2017-03-09T15:53:31Z

Fixed.

Alhadis · 2017-03-10T04:48:44Z

@50Wliu Urgh, I forgot to accommodate arrays when highlighting default values as JavaScript. Something like this will only be partially highlighted:

/** @param {Object} [thing={a: [], b: [0, 2], c: "String"}] [Not Highlighted] [] [] [] */
                                ^ Terminates here

Currently working on a fix, although this is surprisingly tricky to get right...

winstliu · 2017-03-10T05:01:15Z

If it's too difficult/tricky to achieve just revert the embedded highlighting all together.

Alhadis · 2017-03-10T05:03:21Z

No no, so far, so good... it's just that if users get too carried away with really convoluted complex values, they might get flaky-looking highlighting.

Then again, if they're stuffing that much into a default value, what else do you expect? 😀 Heck, this might even discourage an anti-pattern, haha.

Alhadis · 2017-03-10T06:21:11Z

Okay, fixed. Pleeeease don't chide me over the odd-looking way I've implemented this... :D Some repetition was necessary...

Alhadis added 7 commits February 28, 2017 23:02

Torch old JSDoc patterns and refine scope-names

ddc0238

See https://gist.github.com/infininight/9392d1e4408d507a4691e3daba9b3ec9

Remove mention of "embedded" scope in JSDoc blocks

9175375

This could risk triggering incorrect highlighting if a theme colours the contents of interpolated `${ literal }` sequences.

Add JSDoc grammar

d160ece

Remove {type} scope from trailing whitespace

f3d508e

Fix specs and matching of empty /**/ tokens

07439ae

Fix spec-title and add tests for early cutoff

4012f6e

Add specs for embedded JavaScript in @example tags

7274c6c

winstliu added the needs-review label Mar 6, 2017

winstliu reviewed Mar 8, 2017

View reviewed changes

winstliu added under-review and removed needs-review labels Mar 8, 2017

Apply revisions requested in review

0aa448d

Use JavaScript grammar to highlight default values

3508333

winstliu reviewed Mar 9, 2017

View reviewed changes

Alhadis added 2 commits March 10, 2017 02:38

Remove needlessly possessive quantifiers

c422015

Highlight {@inline tags} in default @param values

70a6c53

winstliu merged commit 2ba9a98 into atom:master Mar 9, 2017

andrewstart mentioned this pull request Mar 27, 2017

freeze after opening the following snippet microsoft/TypeScript-TmLanguage#435

Closed

lildude mentioned this pull request May 3, 2017

Release v5.0.9 github-linguist/linguist#3597

Merged

		runs ->
		grammar = atom.grammars.grammarForScopeName("source.js")

Decouple JSDoc from JavaScript grammar / Highlight examples #496

Decouple JSDoc from JavaScript grammar / Highlight examples #496

Uh oh!

Conversation

Alhadis commented Mar 3, 2017

Description of the Change

Alternate Designs / Thoughts

Benefits

Possible Drawbacks

Uh oh!

Alhadis commented Mar 3, 2017

Uh oh!

winstliu commented Mar 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

winstliu commented Mar 8, 2017

Uh oh!

Alhadis commented Mar 9, 2017

Uh oh!

Alhadis commented Mar 9, 2017

Uh oh!

winstliu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Alhadis commented Mar 9, 2017

Uh oh!

Alhadis commented Mar 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

winstliu commented Mar 10, 2017

Uh oh!

Alhadis commented Mar 10, 2017

Uh oh!

Alhadis commented Mar 10, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Alhadis commented Mar 10, 2017 •

edited

Loading