Improve highlighting of embedded @example source #497

Alhadis · 2017-03-10T06:20:32Z

Follow-up of an issue I noticed after the responsible PR was merged.

Cons: It's left the patterns looking hairier than I am.

Pros: The output looks good enough to eat:

Top: My hand-rolled, deliberately-plain and undistracting syntax theme
Bottom: The beautifully-elegant, almost palpably paper-like DuoTone Light

EDIT 14/03: Okay, a few more additions... I feel bad piling them onto an unrelated PR, but I figure it'll probably get things reviewed quicker (since this already needs revisiting).

HTML highlighting added to embedded <caption> tags:

<caption> tags will now match on a newline following the opening @example tag

3 @api and @internal added to recognised JSDoc tags; see comment below

Alhadis · 2017-03-10T06:23:34Z

@50Wliu I also noticed JSDoc drops any text that's touching the closing bracket of a default @param value, so I've highlighted it as an error:

Specs included, of course. =)

Alhadis · 2017-03-10T12:18:10Z

Okay, aaand I noticed incomplete highlighting of @see {@link http://url.com/} (only the opening { was being highlighted). I remember I meant to get back to that... sorry! :( I've added a fix for it to this PR.

Alhadis · 2017-03-11T05:44:46Z

@50Wliu Is it okay to add @api and @internal as highlighted tags? They're not part of JSDoc, but they are used in-the-wild for similar API-generation systems.

E.g.:

@internal
@internal (Grep`"@internal" in the page)
@api (All throughout MochaJS's codebase)

EDIT: Never mind, I figure if it isn't appropriate, you'd bring it up in review.

@example

* Embedded HTML is now highlighted correctly * <caption> tags will match after the opening @example line

Not part of the official JSDoc spec, yet occasionally used in-the-wild.

winstliu · 2017-03-17T22:11:04Z

grammars/jsdoc.cson

-        'end': '\\]|(?=\\*/)'
+        'beginCaptures':
+          '0':
+            'name': 'variable.other.jsdoc'


Use name here instead of contentName if the begin and end captures should get the same scopes.

It's to keep invalid.illegal.syntax.jsdoc from being nested inside variable.other.jsdoc. For themes that prioritise the former over the latter, we don't want invalid text being highlighted as valid.

In a theme which doesn't highlight invalid.illegal.syntax.jsdoc:

Subtle detail, I admit.

I don't like this. I feel like it introduces complexities in the grammar, and I've noticed that while it's unnoticeable while writing code, internally the scopes are still separated (such as in specs).

As you know I really value correct highlighting, but I think in this case it's better to use name.

As you wish.

winstliu · 2017-03-17T22:13:35Z

grammars/jsdoc.cson

+  'default-value':
+    'patterns': [
+      # Double-quoted string
+      {


Why are we special-casing strings?

To stay consistent with JavaScript. Furthermore, some themes might colour double-quoted strings differently to single-quoted strings (such as seti-syntax).

Same thing here. The double string.quoted.double.js capture/contentName feel extremely hacky to me, and is actually why it's taken me so long to get back to reviewing this PR.

Is there any other way to do this without the double capture?

Would you prefer I simply embed source.js in the captured match, then? Unless you want no string highlighting whatsoever...

Wait, scratch that suggestion. That's only gonna make things even more complex, because care will need to be taken to stop source.js from consuming the closing quote-character.

Is there any logical, practical reasoning for this, other than the old "it seems too hacky to me"...? Heck, for all you know, you could be ditching this grammar in six months in favour of Tree-Sitter or Bush-Croucher or something... =)

FYI, this isn't that different from the hacks I used in atom/language-xml#54 to circumvent runaway highlighting with </script> tags and stuff. Heck, that PR's hacks are a hell of a lot worse, and the results aren't even gonna be appreciated on the level that consistent JSDoc highlighting will.

That also happens to be why I haven't finished reviewing the XML PR. This probably won't make it into either the linguist or Atom releases as I'll need time to reflect and think about your questions.

And also, would it be feasible to include all of source.js in here after atom/first-mate#90 is merged?

@50Wliu If it's really this much of an issue, I'll drop it for you. I'm not going to let this be delayed on something this trivial.

Hold on.

Alright, it's done. I've merged both string-matching blocks, and used string.quoted.js for the applied scope.

If there's anything else that's blocking this, please speak up.

EDIT: Oh, and regarding your question: I believe so, yes. It would also warrant a full (and cleaner/simpler) rewrite of the XML/SVG PR, which I'm not that proud of anyway (I recently looked through the expressions I wrote last year for it, and couldn't figure out what the hell I was thinking...)

winstliu · 2017-03-17T22:15:42Z

grammars/jsdoc.cson

+          }
+        ]
+      }
+      {


What exactly is this for?

Stuff like @param {Object} [value = {key: [ [0], [1] ] }] Description

So will this be needed after language-javascript switches to using begin/end captures for square brackets?

Yes, because it's used to balance all that weird, noisy syntax Google use for Closure Compiler tags, which combine < [ { brackets with nesting and all sorts of weird-looking rubbish.

Alhadis · 2017-04-05T14:38:44Z

@50Wliu Any chance we could get this looked at soon?

Linguist is probably due for another release soon, so it'd be great to get this working across GitHub...

I wouldn't be nagging if I didn't believe there isn't much left to do here. =)

winstliu · 2017-04-05T14:52:58Z

I was actually reminding myself to look at this again last night, also because a new Atom release is approaching.

I have mostly refrained from doing so because it's a surprisingly large PR, which I haven't had the appetite to review. I'll try to get to it soon, but like usual, no guarantees.

Alhadis · 2017-04-05T14:54:58Z

If it helps, I kept my last commits atomic and documented to ameliorate any hassle. =)

winstliu

Is there any way at all to get rid of the capture + contentName thing you're doing?

winstliu · 2017-04-05T17:23:12Z

grammars/jsdoc.cson

-        'end': '\\]|(?=\\*/)'
+        'beginCaptures':
+          '0':
+            'name': 'variable.other.jsdoc'


I don't like this. I feel like it introduces complexities in the grammar, and I've noticed that while it's unnoticeable while writing code, internally the scopes are still separated (such as in specs).

As you know I really value correct highlighting, but I think in this case it's better to use name.

winstliu · 2017-04-05T17:25:45Z

grammars/jsdoc.cson

+  'default-value':
+    'patterns': [
+      # Double-quoted string
+      {


Same thing here. The double string.quoted.double.js capture/contentName feel extremely hacky to me, and is actually why it's taken me so long to get back to reviewing this PR.

Is there any other way to do this without the double capture?

Requested in review. See: #497 files/7969d32c25d1ea4a2be45606a5b6c03f1a191ddf#r109976644 (comment)

Alhadis · 2017-04-09T14:51:22Z

@50Wliu Any chance of this making the next Atom release now that I've simplified the pattern/scope-matching?

winstliu · 2017-04-09T16:12:42Z

I will take a look on Monday.

Alhadis · 2017-04-10T18:15:21Z

How's Monday going?

It's Tuesday here, hahah. Was already Monday here when you posted that, too. 😀

winstliu · 2017-04-11T01:04:35Z

grammars/jsdoc.cson

+        'name': 'source.embedded.js'
+        'captures':
+          '0':
+            'patterns': [


Curious what this is for.

Couldn't figure out what was causing it, and it was giving me the shits, so I added a different rule.

Can you give me a code example of what caused you to add this rule?

No, sorry, I've completely forgotten what I was even working on when I encountered this.

Are you waiting for me to remove this rule, or...?

I think you've realized by now that you and I sometimes disagree on what is shippable. I apologize for the long delays between reviews and comments; that's something I need to work on when something like this occurs.

If you would really like Linguist to include the new JSDoc highlighting, I would encourage you to submit a (hopefully temporary) PR that eliminates the embedded JS highlighting. Then, we can continue discussing.

I've been trying out different methods to avoid the scope-chopping that's occurring here. I've seen some success but it'll take more time and I don't want to delay your Linguist release or have it ship with buggy highlighting.

I think you've realized by now that you and I sometimes disagree on what is shippable.

What you're neglecting to do is explain to me why. So far, it seems like a case of "it doesn't feel right to me". That's what's disconcerting to me. I can handle being disagreed with... but not in the absence of a reason why.

Once I'm back in front of a computer, I'm replacing this grammar with a fork until you've come to a resolution.

Right, never mind. I explained the issue to Linguist's maintainer and he believes the best the thing to do at this point is to avoid updating the JavaScript grammar with the next release, so the grammar's current state won't affect GitHub.

Fix this whenever. No hurry now.

Doesn't look like this will be sorted any time soon, so I've undone my removal of the embedded HTML highlighting.

Yeah. So I didn't explain why because it was getting late last night but I still wanted to respond to let you know I got the notification.

Here's why I think this PR is unmergeable as-is, or even with an explanatory comment.

These changes result in certain parts of language-javascript being spliced up such that the pattern begins matching but is interrupted before the end match.

Therefore you had to add some out-of-context rules in an attempt to balance out the mismatched rules. This is very prone to breakage as scopes or matches change.

Inconsistent scopes. Let's look at https://github.com/atom/language-javascript/pull/497/files#diff-c4166255a13782b6557ec1da0f4a6ef3R467 for an example. Why is "b" not tokenized as embedded JavaScript, while it should be? The whole default value should be embedded JS, yet there are portions when that scope disappears. This of course is partly due to my insistence that double-matching should be avoided where possible, but that also brings me to my next point:

Duplication of rules found in javascript.cson. Again, this is very hard to keep in sync.

Considerably, as you described it, "hairier" patterns for comparably small highlighting gains.

Overall, after reviewing this PR I am unconvinced that embedded Javascript highlighting for default values is worth it if it cannot be done in a clean manner that is easy to understand and requires minimal maintenance upkeep.

winstliu · 2017-04-11T01:07:11Z

spec/jsdoc-spec.coffee

-
+    describe "HTML captions", ->
+      ###
+        NOTE: Loading the HTML grammar triggers an inexplicable case of infinite recursion during the following two specs.


Hmm. That is quite odd.

winstliu · 2017-04-11T01:08:09Z

9:07pm over here 😀. I'm going to take a look at the HTML recursion.

winstliu · 2017-04-11T01:11:39Z

Also: I think I'm going to change the update schedule for grammars in Atom. Instead of updating all the grammars right before a new release, I'm going to update them right after, so that they get the full benefit of Atom's release pattern. That also means that there will be very minimal language updates for 1.17.

Alhadis · 2017-04-11T01:14:26Z

Eh... okay...

Wasn't the first buggy JSDoc PR I submitted merged in a release used by the next Atom version?

winstliu · 2017-04-11T01:24:33Z

Nope: #496 hasn't been included in a language-javascript release yet.

Also, I can reproduce the infinite recursion in a production environment. The repro case is as simple as having the caption element, as in the first spec, and then trying to disable and then re-enable language-html. That will need to be fixed.

EDIT: Or even just reloading the window?
EDIT2: Yup, reloading the window will reliably trigger it if you're using the second spec

Alhadis · 2017-04-11T01:32:15Z

Nope: #496 hasn't been included in a language-javascript release yet.

Well, thankfully Linguist doesn't need a tagged release to pick up the latest changes to a grammar. :D

As for Atom, I'm not fussed about getting this shipped soon. I have enough to worry about with keeping File-Icons stable until the 5 PRs are dealt with.

Alhadis · 2017-04-20T08:47:18Z

@50Wliu There's very likely going to be a new Linguist release soon. What else is there that I have to do?

I'd rather not have the bugs from the first PR plague every documented JS file throughout GitHub.

winstliu · 2017-04-20T13:41:04Z

The infinite recursion issue with language-html needs to be fixed.

Alhadis · 2017-04-20T13:46:30Z

Would you prefer I remove the embedded HTML highlighting for the time being, then?

winstliu · 2017-04-20T13:53:03Z

I'm not opposed to that. It can always be added later.

Alhadis · 2017-04-20T13:54:00Z

Precisely my line of thinking as well. Hold on.

Alhadis · 2017-04-20T14:05:27Z

Righto, done. =)

Damn strange I can't reproduce the recursion error outside of the spec-runner, though.

winstliu · 2017-04-20T14:33:41Z

Did you see my reproduction steps above?

Alhadis · 2017-04-20T14:34:49Z

Ah, I missed the part about disabling and reenabling. Sorry, never mind.

winstliu · 2017-04-20T14:40:12Z

I will try to give a final review within the next few days.

winstliu · 2017-04-21T17:07:35Z

grammars/jsdoc.cson

+        'end': '\\1|(?=\\*/)'
+        'endCaptures':
+          '0':
+            'name': 'punctuation.definition.string.end.js'


I noticed that in bd688cc you simplified the string rules, but shouldn't name still differentiate between single and double quoted strings?

How am I expected to do that without duplicating the rule?

I'm fine with the rule being duplicated. Just like how it already is in the main javascript grammar (

language-javascript/grammars/javascript.cson

Lines 1338 to 1375 in f470e13

'begin': '\''

'beginCaptures':

'0':

'name': 'punctuation.definition.string.begin.js'

'end': '\''

'endCaptures':

'0':

'name': 'punctuation.definition.string.end.js'

'name': 'string.quoted.single.js'

'patterns': [

{

'include': '#string_escapes'

}

{

'match': "[^']*[^\\n\\r'\\\\]$"

'name': 'invalid.illegal.string.js'

}

]

}

{

'begin': '"'

'beginCaptures':

'0':

'name': 'punctuation.definition.string.begin.js'

'end': '"'

'endCaptures':

'0':

'name': 'punctuation.definition.string.end.js'

'name': 'string.quoted.double.js'

'patterns': [

{

'include': '#string_escapes'

}

{

'match': '[^"]*[^\\n\\r"\\\\]$'

'name': 'invalid.illegal.string.js'

}

]

).

winstliu · 2017-04-21T17:08:25Z

grammars/jsdoc.cson

+        'name': 'source.embedded.js'
+        'captures':
+          '0':
+            'patterns': [


Can you give me a code example of what caused you to add this rule?

This reverts commit da0b7b2.

The current grammar has a known issue and is pending the fix in atom/language-javascript#497

* Update all grammars * Update atom-language-clean grammar to match * Don't update reason grammer There seems to be a problem with the 1.3.5 release in that the conversion isn't producing a reason entry so doesn't match whats in grammar.yml * Bump version to 5.0.9 * Update grammars * Don't update javascript grammar The current grammar has a known issue and is pending the fix in atom/language-javascript#497

Alhadis · 2017-05-03T15:27:27Z

I won't waste your time anymore. Sorry.

I'll leave it to you to revert the embedded highlighting in my other PR.

Alhadis · 2017-05-03T15:30:54Z

Going forward, I think the best thing for me to do is refrain from submitting future PRs to grammar packages, as it's obvious our opinions of grammar structure are too different for these things to be tolerable for either of us.

Alhadis added 4 commits March 10, 2017 16:27

Fix boundary-matching with arrays as default values

b063622

Fix tokenisation of string literals in @param defaults

a9c95c6

Add specs for matching escape sequences in strings

1875412

Add additional specs for invalid descriptions

3f21285

Fix tokenisation of "@see {@link …}"

7372282

winstliu added the needs-review label Mar 10, 2017

Alhadis added 2 commits March 14, 2017 21:21

Improve tokenisation of <caption> tags in @example

a8f1e4e

* Embedded HTML is now highlighted correctly * <caption> tags will match after the opening @example line

Add @api and @internal to recognised JSDoc tags

7969d32

Not part of the official JSDoc spec, yet occasionally used in-the-wild.

winstliu reviewed Mar 17, 2017

View reviewed changes

winstliu reviewed Apr 5, 2017

View reviewed changes

Alhadis added 2 commits April 6, 2017 03:34

Amend scopes used to highlight invalid param names

eb04696

Requested in review. See: #497 files/7969d32c25d1ea4a2be45606a5b6c03f1a191ddf#r109976644 (comment)

Simplify tokenisation of quoted strings in JSDoc

bd688cc

winstliu reviewed Apr 11, 2017

View reviewed changes

Remove embedded HTML highlighting from captions

da0b7b2

winstliu reviewed Apr 21, 2017

View reviewed changes

Alhadis added 2 commits April 22, 2017 16:52

Differentiate between double and single-quoted strings

dab4bb9

Revert removal of embedded HTML highlighting

840c818

This reverts commit da0b7b2.

lildude added a commit to github-linguist/linguist that referenced this pull request May 3, 2017

Don't update javascript grammar

2620c6e

The current grammar has a known issue and is pending the fix in atom/language-javascript#497

winstliu mentioned this pull request May 24, 2017

Re-implement embedded JSDoc JavaScript highlighting #512

Merged

	'begin': '\''
	'beginCaptures':
	'0':
	'name': 'punctuation.definition.string.begin.js'
	'end': '\''
	'endCaptures':
	'0':
	'name': 'punctuation.definition.string.end.js'
	'name': 'string.quoted.single.js'
	'patterns': [
	{
	'include': '#string_escapes'
	}
	{
	'match': "[^']*[^\\n\\r'\\\\]$"
	'name': 'invalid.illegal.string.js'
	}
	]
	}
	{
	'begin': '"'
	'beginCaptures':
	'0':
	'name': 'punctuation.definition.string.begin.js'
	'end': '"'
	'endCaptures':
	'0':
	'name': 'punctuation.definition.string.end.js'
	'name': 'string.quoted.double.js'
	'patterns': [
	{
	'include': '#string_escapes'
	}
	{
	'match': '[^"]*[^\\n\\r"\\\\]$'
	'name': 'invalid.illegal.string.js'
	}
	]

Improve highlighting of embedded @example source #497

Improve highlighting of embedded @example source #497

Uh oh!

Conversation

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

Uh oh!

Uh oh!

Alhadis commented Mar 10, 2017

Uh oh!

Alhadis commented Mar 10, 2017

Uh oh!

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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!

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

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 Apr 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

winstliu commented Apr 5, 2017

Uh oh!

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

Uh oh!

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!

Alhadis commented Apr 9, 2017

Uh oh!

winstliu commented Apr 9, 2017

Uh oh!

Alhadis commented Apr 10, 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

Alhadis commented Mar 10, 2017 •

edited

Loading

Alhadis commented Mar 11, 2017 •

edited

Loading

Alhadis Mar 20, 2017 •

edited

Loading

Alhadis Apr 7, 2017 •

edited

Loading

Alhadis commented Apr 5, 2017 •

edited

Loading

Alhadis commented Apr 5, 2017 •

edited

Loading

Alhadis May 3, 2017 •

edited

Loading

Alhadis May 3, 2017 •

edited

Loading

winstliu commented Apr 11, 2017 •

edited

Loading

Alhadis commented Apr 20, 2017 •

edited

Loading