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

Add support for multi-line types in jsdoc. #515

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

Conversation

ribrdb
Copy link

@ribrdb ribrdb commented Jun 3, 2017

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • All new code requires tests to ensure against regressions

Description of the Change

Support for multi line types in jsdoc.
See microsoft/TypeScript-TmLanguage#467

And here's an example file with multi-line types:
https://github.com/google/closure-library/blob/master/closure/goog/dom/animationframe/animationframe.js#L61

Alternate Designs

I don't know of any.

Benefits

Multi line types are correctly highlighted.

Possible Drawbacks

Unterminated types are not recognized as invalid. I couldn't figure out any way to maintain that. However you can clearly see that the type runs on longer than you're expecting if you forget the closing }, so I don't think that's a major issue.

Applicable Issues

@winstliu
Copy link
Contributor

winstliu commented Jun 8, 2017

I think it's worth tokenizing {{ as a single punctuation.definition.bracket.curly.begin.jsdoc (same for ending brackets), and maybe even applying specific rules when that is encountered. I believe that should allow us to maintain the invalid highlighting. Thoughts?

@@ -936,6 +936,36 @@ describe "JSDoc grammar", ->
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', 'variable.other.jsdoc']

{tokens} = grammar.tokenizeLine('/** @param {\nnumber\n} variable this is the description */')
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of using \n, could you please use grammar.tokenizeLines and multiline quotes? See https://github.com/atom/language-javascript/blob/master/spec/jsdoc-spec.coffee#L1300 for an example.

@ribrdb
Copy link
Author

ribrdb commented Jun 8, 2017

{{ seems to be the most common reason to split a type across lines, but I don't think it's the only one. For example a function type with many arguments or a union type with many alternatives can also get long.

@@ -410,6 +410,10 @@
}
]
}
{
'match': '^\\s*\\*(?!/)'
'name': 'comment.block.documentation.jsdoc'

Choose a reason for hiding this comment

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

Comment. #114

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure what you mean. Do you want me to add a comment describing this rule? I don't see what this has to do with issue 114.

Copy link
Contributor

Choose a reason for hiding this comment

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

We occasionally get spammers due to the popularity of the Atom project. I think this comment is safe to ignore.

Copy link
Contributor

Choose a reason for hiding this comment

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

Wrong scope-name. It should be punctuation.section.continuation.comment.js, or at least just punctuation.section.comment.js. See here.

But this pattern isn't enclosing any content of its own, so it's incorrect to scope it as a comment.

@winstliu
Copy link
Contributor

/cc @Alhadis

@Alhadis
Copy link
Contributor

Alhadis commented Jun 20, 2017

My approach would've been to add a separate rule for multiline types, prepended to the JSDoc grammar's #type rule. Something like this:

	'type':
		'patterns': [
			{
				# {{ …Multiline type… }}
				'name': 'meta.multiline-type.jsdoc'
				'begin': '\\G\\{\\{'
				'end':   '\\}\\}|(?=\\*/)'
				'beginCaptures': 'punctuation.section.bracket.curly.begin.jsdoc'
				'endCaptures':   'punctuation.section.bracket.curly.end.jsdoc'
				'patterns': [ '…etc…' ]
			}
			{
				# {unclosed
				'match': '\\G{(?:[^}*]|\\*[^/}])+$'
				'name': 'invalid.illegal.type.jsdoc'

Dunno exactly what patterns should go in the above example's patterns array, because I've never heard of multiline types until now.

But this approach, while more work, would allow us to keep both the existing error-highlights, and multiline type-definitions.

@ribrdb
Copy link
Author

ribrdb commented Jun 27, 2017

A multi line jsdoc tag is just one that has the 2nd bracket on another line. Often it will start with '{{', but not always. The only way to tell if you've really got an invalid tag is to see if you get the */ before the closing '}'. But the grammar syntax doesn't have any way to do that, since it only does line based matching.

} variable this is the description */
""")
expect(lines[0][5]).toEqual value: '{', scopes: ['source.js', 'comment.block.documentation.js', 'entity.name.type.instance.jsdoc', 'punctuation.definition.bracket.curly.begin.jsdoc']
expect(lines[1][0].value).toMatch(/number/)
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 change this to use the .toEqual style? Same on L1177.

@joined
Copy link

joined commented May 14, 2018

I'd really like to see this happen!

@visj
Copy link

visj commented Mar 1, 2019

Is there an update on this feature?

@duckbrain
Copy link

It seems like the only thing this is waiting for is a style change from code review. Can I submit another PR with those changes to get this moving?

duckbrain added a commit to ec2-software/language-javascript that referenced this pull request May 17, 2019
@jkantr
Copy link

jkantr commented Jun 7, 2019

Apparently the previous comment is no longer accurate, as this needs to be migrated to Tree-sitter grammar to be accepted, is that correct?

Been sort of in limbo now for a few months where I have a feature that works, but without the syntax highlighting to go along with it.... is there help needed to get this resolved?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants