-
Notifications
You must be signed in to change notification settings - Fork 887
Added location, type, and privacy to completed-docs #2095
Conversation
src/rules/completedDocsRule.ts
Outdated
export type All = typeof Rule.ALL; | ||
|
||
export type DocType = All | ||
| typeof Rule.ARGUMENT_CLASSES |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't typeof Rule.ARGUMENT_CLASSES
"string"? In my IDE, DocType resolves to string
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What version of TypeScript are you on? For me (VS Code, TypeScript 2.1.X) typeof Rule.ARGUMENT_CLASSES
correctly resolves to the "classes"
literal.
Historically, before support for discriminated literal types, I liked having this explicit typing - both to inform the user and for future-proofing the type declarations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm using VS Code, TS 2.1.4. So you're saying that const a: Visibility = "s";
causes a compilation error for you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you meant to use const
instead of public static
? In that case, I see the string literal type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, you're totally right. Thanks for the catch! I've switched them to const
.
I think you need to rebase |
Rebase should be good to go. |
I think this may be considered a breaking change if someone already specifies "all" because they'll see new errors. Thoughts? |
Also adds interfaces, enums, types, and a few other doc-able contructs.
Agreed. Changed it to use the current default, so that the new locations are opt-in. For the next TSLint release we could consider expanding default coverage. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
🎽 when it will be released? |
@FDiskas it's available since v4.5. you can find out by yourself by clicking through to the squashed commit: and look at the tags list: |
what's the status? I still can't get it work |
@Viktor-Bredihin what is your tslint.json config, and what are you trying to lint? Can you post a minimum reproduction we can take a look at? What problems are you seeing? |
@JoshuaKGoldberg |
Ah - that's not an issue with this rule. That's an issue with the Webstorm TSLint extension.
|
Also adds interfaces, enums, types, and a few other doc-able contructs.
Fixes #1713.
Lays the groundwork for #1921.