Added tags logic for completed-docs #2415
Conversation
Used a factory instead of public/private static methods. Splitting across files meant I was forced to keep a clean deliniation of responsibilities, showing that the statics was dirty in the first place. #2399.
Inverted the "requirement" concept into an "exclusion" concept. Instead of caring about whether nodes currently _require_ a doc comment (which implies they don't have one at the moment), we now check if there's anything _excluding_ a node from needing to check whether it has a documentation body.
|
:( tests pass locally! Might be a TypeScript version-specific issue? Will take a look soon... Also, I couldn't find existing examples of how to specify an object mapping strings to strings for the config settings (though I have a vague recollection of looking before). Will do a real scan-through soon also... |
|
Looks like it fails for 2.0.10. I think we can remove support for that in 5.0 so I'll remove the test requirement |
Great. Does that mean reviewing this PR will be blocked until 5.0?
Is it ok to wait to standardize these into something other than an |
v5.0 is almost ready, see #2426 |
|
|
||
| public excludes(node: ts.Declaration) { | ||
| const documentationNode = this.getDocumentationNode(node); | ||
| const tagsWithContents = this.parseTagsWithContents(documentationNode.getFullText()); |
There was a problem hiding this comment.
if you want to get the jsdoc comment, you can simply do this:
const [firstChild] = documentationNode.getChildren();
if (firstChild.kind === ts.SyntaxKind.JSDocComment) {
// firstChild is ts.JSDoc
// do stuff with the comment
}For more info on JSDoc, see https://github.com/Microsoft/TypeScript/blob/master/src/compiler/types.ts#L2065
You also don't need to parse it manually with the regex magic below. Typescript's parser already did that for you
There was a problem hiding this comment.
actually there may be more JSDocComments for a given node. but all of them precede nodes of other kinds.
There was a problem hiding this comment.
Sorry @ajafff, I haven't been able to get that to work. Adding an if check for the node being any of the JSDoc kinds in a new visitNode isn't getting triggered. Either I'm missing something obvious or being silly. Are you ok with either taking a look yourself or letting this pass with a followup issue to clean this rule up (switch to the more efficient walker type and use the new APIs instead of type checking)?
There was a problem hiding this comment.
Bump @ajafff ?
There was a problem hiding this comment.
@JoshuaKGoldberg SyntaxWalker#visitNode will never visit any JSDoc nodes, that's why I pointed you to node.getChildren() which does some magic to include jsdoc as the first entries if available.
However, I don't consider this a blocker for the PR. I'm fine with merging this without that change and tweaking it afterwards.
There was a problem hiding this comment.
👍 thanks. If this goes in I'll file a followup bug to do that and refactor this to the newer, more-performant walker style.
src/rules/completedDocsRule.ts
Outdated
| return []; | ||
| } | ||
|
|
||
| return symbol.getDocumentationComment(); |
There was a problem hiding this comment.
starting from [email protected] you don't necessarily need the typechecker for that, see my other comment above
There was a problem hiding this comment.
Awesome thanks, I'll update this...
|
friendly ping @JoshuaKGoldberg |
|
Sorry, been busy. :( should be able to fix this up within the week... Edit: got to it, tests pass, but couldn't resolve ajaff's feedback.. |
# Conflicts: # src/rules/completedDocsRule.ts
The default visibility for class items should always be considered public.
# Conflicts: # src/rules/completedDocsRule.ts
There was a problem hiding this comment.
LGTM
This PR is overwhelming. Especially as its main objective is just to allow configuration which tags do not require a full blown docs comment. That may be caused by your rather verbose coding style. But don't worry, that's not a bad thing.
Fortunately the bulk of the diff is just extracting Requirement -> Exclusion.
| @@ -0,0 +1,42 @@ | |||
| /** | |||
| * @license | |||
| * Copyright 2013 Palantir Technologies, Inc. | |||
Fixes #1921.
Changelog entry
[enhancement] Allow
completed-docsto list doc tags that mark a node as not requiring a documentation body. Tags can also provide a regexp matcher to validate that their contents are docs-valid.