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

JSDoc string literal types #9995

Merged
merged 7 commits into from
Aug 17, 2016
Merged

JSDoc string literal types #9995

merged 7 commits into from
Aug 17, 2016

Conversation

sandersn
Copy link
Member

Fixes #9902

Unfortunately, I didn't find a way to reuse the normal string literal type, so I had to extend the existing JSDoc type hierarchy. Otherwise, this feature is very simple.

sandersn added 2 commits July 27, 2016 13:21
Unfortunately, I didn't find a way to reuse the normal string literal
type, so I had to extend the existing JSDoc type hierarchy. Otherwise,
this feature is very simple.
@sandersn sandersn changed the title Jsdoc string literal types JSDoc string literal types Jul 27, 2016
@@ -346,6 +346,7 @@ namespace ts {
JSDocTypedefTag,
JSDocPropertyTag,
JSDocTypeLiteral,
JSDocStringLiteralType,

Copy link
Contributor

Choose a reason for hiding this comment

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

You may need to update the LastJSDocNode and LastJSDocTagNode pointers

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch!

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated

@sandersn
Copy link
Member Author

sandersn commented Aug 3, 2016

@DanielRosenwasser do you mind taking a look since you did the rest of string literal types?

* @param {'literal' | number} p4
*/
function f(p1, p2, p3, p4) {
return p1 + p2 + p3 + p4 + '.';
Copy link
Member

Choose a reason for hiding this comment

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

Does this test really do anything apart from test baselines? Why not write some tests that have observable characteristics from a usage standpoint?

For instance, string completion when calling something that takes a union type of string literals.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea to add a string-literal-specific test. I didn't even know that we offered completions in strings.

This test checks that the jsdoc parser gets the types right. It's in a JavaScript file so the parameter types would otherwise be any. That will produce a lot of observable differences in parameter help and quick info. But I trust those work if the checker can get the types right.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.
Hilariously, @Filename is case-sensitive in fourslash tests, but not compiler and conformance tests.
Also, Codeflow has two Give Feedback buttons, and they are not the same -- one is the inverse colour of the other.
OK, I'm done complaining now.

@sandersn
Copy link
Member Author

sandersn commented Aug 4, 2016

All right, now that I merged with master, JSDoc supports all literal types.

case SyntaxKind.StringLiteral:
case SyntaxKind.NumericLiteral:
case SyntaxKind.TrueKeyword:
case SyntaxKind.FalseKeyword:
Copy link
Member Author

Choose a reason for hiding this comment

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

need NeverKeyword and UndefinedKeyword here too

Copy link
Contributor

Choose a reason for hiding this comment

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

and NullKeyword as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, those are not literal types, it turns out. In the interests of getting this PR in, I'll do those separately.

Copy link
Member Author

Choose a reason for hiding this comment

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

They probably need their own special handling (see the lines preceding the diff in checker)

@mhegazy
Copy link
Contributor

mhegazy commented Aug 16, 2016

Can you update the switch statement for parsing, otherwise 👍

@sandersn sandersn merged commit c218d37 into master Aug 17, 2016
@sandersn sandersn deleted the jsdoc-string-literal-types branch August 17, 2016 17:26
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
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.

5 participants