Skip to content
This repository was archived by the owner on Dec 15, 2022. It is now read-only.

Improve highlighting of embedded @example source #497

Closed
wants to merge 12 commits into from
Closed

Improve highlighting of embedded @example source #497

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
b063622
Fix boundary-matching with arrays as default values
Alhadis Mar 10, 2017
a9c95c6
Fix tokenisation of string literals in @param defaults
Alhadis Mar 10, 2017
1875412
Add specs for matching escape sequences in strings
Alhadis Mar 10, 2017
3f21285
Add additional specs for invalid descriptions
Alhadis Mar 10, 2017
7372282
Fix tokenisation of "@see {@link …}"
Alhadis Mar 10, 2017
a8f1e4e
Improve tokenisation of <caption> tags in @example
Alhadis Mar 14, 2017
7969d32
Add @api and @internal to recognised JSDoc tags
Alhadis Mar 14, 2017
eb04696
Amend scopes used to highlight invalid param names
Alhadis Apr 5, 2017
bd688cc
Simplify tokenisation of quoted strings in JSDoc
Alhadis Apr 7, 2017
da0b7b2
Remove embedded HTML highlighting from captions
Alhadis Apr 20, 2017
dab4bb9
Differentiate between double and single-quoted strings
Alhadis Apr 22, 2017
840c818
Revert removal of embedded HTML highlighting
Alhadis May 3, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
209 changes: 166 additions & 43 deletions grammars/jsdoc.cson
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
'3':
'name': 'constant.language.access-type.jsdoc'
'match': '''(?x)
((@)access)
((@)(?:access|api))
\\s+
(private|protected|public)
\\b
Expand Down Expand Up @@ -78,21 +78,16 @@
'2':
'name': 'punctuation.definition.block.tag.jsdoc'
'patterns': [
{
# Match to prevent leading asterisk being highlighted as JS
'match': '^\\s\\*\\s+'
}
{
# Leading <caption>…</caption> before example
'begin': '\\G(<)caption(>)'
'begin': '(<)caption(>)'
'beginCaptures':
'0':
'name': 'entity.name.tag.inline.jsdoc'
'1':
'name': 'punctuation.definition.bracket.angle.begin.jsdoc'
'2':
'name': 'punctuation.definition.bracket.angle.end.jsdoc'
'contentName': 'constant.other.description.jsdoc'
'end': '(</)caption(>)|(?=\\*/)'
'endCaptures':
'0':
Expand All @@ -101,18 +96,38 @@
'name': 'punctuation.definition.bracket.angle.begin.jsdoc'
'2':
'name': 'punctuation.definition.bracket.angle.end.jsdoc'
'name': 'meta.caption.jsdoc'
'patterns': [
{
# Keep leading asterisks highlighted as comments in multiline captions
'match': '^\\s\\*\\s+'
}
{
# Highlight contents as embedded HTML
'match': '(?:[^<*]|\\*[^/]|<(?!/caption>))+'
'name': 'constant.other.description.jsdoc'
'captures':
'0':
'patterns': [
{
'include': 'text.html.basic'
}
]
}
]
}
{
# Highlighted JavaScript example
'match': '[^\\s@*](?:[^*]|\\*[^/])*'
'captures':
'0':
'name': 'source.embedded.js'
'patterns': [
{
'include': 'source.js'
}
]
# Make sure <caption> only matches once in each @example
'begin': '(?<=</caption>)'
'end': '(?=@|\\*/)'
'patterns': [
{
'include': '#example-js'
}
]
}
{
'include': '#example-js'
}
]
}
Expand Down Expand Up @@ -155,7 +170,15 @@
|
# JSDoc namepath
(
(?!https?://)
(?!
# Avoid matching bare URIs (also acceptable as links)
https?://
|
# Avoid matching {@inline tags}; we match those below
(?:\\[[^\\[\\]]*\\])? # Possible description [preceding]{@tag}
{@(?:link|linkcode|linkplain|tutorial)\\b
)
# Matched namepath
(?:[^@\\s*/]|\\*[^/])+
)
)
Expand Down Expand Up @@ -252,28 +275,24 @@
'name': 'variable.other.jsdoc'
}
{
'name': 'variable.other.jsdoc'
'begin': '\\['
'end': '\\]|(?=\\*/)'
'end': '\\]((?:[^*\\s]|\\*[^\\s/])+)?|(?=\\*/)'
'endCaptures':
'1':
'name': 'invalid.illegal.syntax.jsdoc'
'name': 'variable.other.jsdoc'
'patterns': [
{
'match': '(=)((?:[^\\]*]|\\*[^/])*)'
'captures':
'1':
'begin': '='
'beginCaptures':
'0':
'name': 'keyword.operator.assignment.jsdoc'
'2':
'name': 'source.embedded.js'
'patterns': [
{
'include': '#inline-tags'
}
{
'include': 'source.js'
}
]
}
{
'include': '#brackets'
'end': '(?=\\]|\\*/)'
'patterns': [
{
'include': '#default-value'
}
]
}
{
'include': '#quotes'
Expand Down Expand Up @@ -369,16 +388,16 @@
'1':
'name': 'punctuation.definition.block.tag.jsdoc'
'match': '''(?x) (@)
(?:abstract|access|alias|arg|argument|async|attribute|augments|author|beta|borrows|bubbles
(?:abstract|access|alias|api|arg|argument|async|attribute|augments|author|beta|borrows|bubbles
|callback|chainable|class|classdesc|code|config|const|constant|constructor|constructs|copyright
|default|defaultvalue|define|deprecated|desc|description|dict|emits|enum|event|example|exception
|exports?|extends|extension(?:_?for)?|external|externs|file|fileoverview|final|fires|for|func
|function|global|host|ignore|implements|implicitCast|inherit[Dd]oc|inner|instance|interface|kind
|lends|license|listens|main|member|memberof!?|method|mixes|mixins?|modifies|module|name|namespace
|noalias|nocollapse|nocompile|nosideeffects|override|overview|package|param|preserve|private|prop
|property|protected|public|read[Oo]nly|record|require[ds]|returns?|see|since|static|struct|submodule
|summary|suppress|template|this|throws|todo|tutorial|type|typedef|unrestricted|uses|var|variation
|version|virtual|writeOnce)
|function|global|host|ignore|implements|implicitCast|inherit[Dd]oc|inner|instance|interface
|internal|kind|lends|license|listens|main|member|memberof!?|method|mixes|mixins?|modifies|module
|name|namespace|noalias|nocollapse|nocompile|nosideeffects|override|overview|package|param|preserve
|private|prop|property|protected|public|read[Oo]nly|record|require[ds]|returns?|see|since|static
|struct|submodule|summary|suppress|template|this|throws|todo|tutorial|type|typedef|unrestricted
|uses|var|variation|version|virtual|writeOnce)
\\b
'''
'name': 'storage.type.class.jsdoc'
Expand Down Expand Up @@ -411,6 +430,110 @@
]
}
]
# Default @param value, highlighted as JavaScript
'default-value':
'patterns': [
# Double-quoted string
{
'begin': '"'
'beginCaptures':
'0':
'name': 'punctuation.definition.string.begin.js'
'end': '"|(?=\\*/)'
'endCaptures':
'0':
'name': 'punctuation.definition.string.end.js'
'name': 'string.quoted.double.js'
'patterns': [
{
'match': '\\\\(?:[^*]|\\*(?!/))'
'name': 'constant.character.escape.js'
}
]
}
# Single-quoted string
{
'begin': '\''
'beginCaptures':
'0':
'name': 'punctuation.definition.string.begin.js'
'end': '\'|(?=\\*/)'
'endCaptures':
'0':
'name': 'punctuation.definition.string.end.js'
'name': 'string.quoted.single.js'
'patterns': [
{
'match': '\\\\(?:[^*]|\\*(?!/))'
'name': 'constant.character.escape.js'
}
]
}
{
# Nested arrays matched to keep brackets balanced
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What exactly is this for?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

'begin': '(\\[)'
'beginCaptures':
'0':
'name': 'source.embedded.js'
'1':
# NOTE: Should ideally be "punctuation.definition.bracket.square.begin.js"
# We're using the old/"incorrect" scope to ensure consistent highlighting.
'name': 'meta.brace.square.js'
'end': '(\\])|(?=\\*/)'
'endCaptures':
'0':
'name': 'source.embedded.js'
'1':
'name': 'meta.brace.square.js'
'patterns': [
{
'include': '#default-value'
}
]
}
# Chunk of JS source (excluding nested arrays; see above). Note we "clamp" the
# imported highlighting within a fixed range to stop runaway problems with any
# begin/end patterns the JavaScript grammar matches.
{
'match': '(?:[^"\'\\[\\]*]|\\*[^/])+'
'name': 'source.embedded.js'
'captures':
'0':
'patterns': [
# Unmatched } after a string literal: `"string"}`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious what this is for.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@Alhadis Alhadis May 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@Alhadis Alhadis May 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
'match': '}'
'name': 'meta.brace.curly.js'
}
{
'include': '#inline-tags'
}
{
'include': 'source.js'
}
]
}
]
# Highlighted JavaScript for @example tag
'example-js':
'patterns': [
{
# Match to prevent leading asterisk being highlighted as JS
'match': '^\\s\\*\\s+'
}
{
# "Clamped" line of JS source
'match': '[^\\s@*](?:[^*]|\\*[^/])+'
'captures':
'0':
'name': 'source.embedded.js'
'patterns': [
{
'include': 'source.js'
}
]
}
]
'inline-tags':
'patterns': [
{
Expand Down
Loading