Avoid duplicates when finding jsdoc #24086
Merged
+10
−11
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
1. Add a cheap assert for detecting duplicates. getJSDocTags must find either [1] 0 or 1 comments or [2] the first two comments must not be identical. This does not always find duplicates for nodes with 3 or more comments, but these nodes are exceptionally rare. This assert fired for over 20 of the around 250 tests we have that retrieve JSDoc at all. So I fixed the asserts in [2] and [3]. 2. There were overlapping cases from calls to getSourceOfAssignment and getSpecialPropertyAssignment. getSpecialPropertyAssignment is too restrictive, but was in the correct location (parent vs parent.parent), so I removed the getSourceOfAssignment call and replaced the getSpecialPropertyAssignment calls with a less restrictive check. 3. When a node's parent is a PropertyDeclaration, getJSDocCOmmentsAndTags would check the parent for jsdoc. But when the *node* is a PropertyDeclaration, getJSDocCommentsAndTags would use the jsdoc from the initializer. This second case is useful to make sure that property declarations get all their jsdoc, but results in duplicates for their initializers. I couldn't think of a better fix than tracking the previous node in the recursive lookup of getJSDocCommentsAndTags, which is a little clunky.
|
Build failed because of lint, which is fixed now. |
mhegazy
approved these changes
May 15, 2018
sandersn
added a commit
that referenced
this pull request
May 16, 2018
* Avoid duplicates when finding jsdoc 1. Add a cheap assert for detecting duplicates. getJSDocTags must find either [1] 0 or 1 comments or [2] the first two comments must not be identical. This does not always find duplicates for nodes with 3 or more comments, but these nodes are exceptionally rare. This assert fired for over 20 of the around 250 tests we have that retrieve JSDoc at all. So I fixed the asserts in [2] and [3]. 2. There were overlapping cases from calls to getSourceOfAssignment and getSpecialPropertyAssignment. getSpecialPropertyAssignment is too restrictive, but was in the correct location (parent vs parent.parent), so I removed the getSourceOfAssignment call and replaced the getSpecialPropertyAssignment calls with a less restrictive check. 3. When a node's parent is a PropertyDeclaration, getJSDocCOmmentsAndTags would check the parent for jsdoc. But when the *node* is a PropertyDeclaration, getJSDocCommentsAndTags would use the jsdoc from the initializer. This second case is useful to make sure that property declarations get all their jsdoc, but results in duplicates for their initializers. I couldn't think of a better fix than tracking the previous node in the recursive lookup of getJSDocCommentsAndTags, which is a little clunky. * Fix lint; remove new context parameter * Update importJsNodeModule3 with fix
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This assert fired for over 20 of the around 250 tests we have that retrieve JSDoc at all. So I fixed the asserts in [2] and [3]. However, no tests failed with or without duplicates, so I think our existing jsdoc code is resilient to duplicates.
There were overlapping cases from calls to getSourceOfAssignment and getSpecialPropertyAssignment. getSpecialPropertyAssignment is too restrictive, but was in the correct location (parent vs parent.parent), so I removed the getSourceOfAssignment call and replaced the getSpecialPropertyAssignment calls with a less restrictive check.
When a node's parent is a PropertyDeclaration, getJSDocCOmmentsAndTags would check the parent for jsdoc. But when the node is a PropertyDeclaration, getJSDocCommentsAndTags would use the jsdoc from the initializer. This second case is useful to make sure that property declarations get all their jsdoc, but results in duplicates for their initializers. I couldn't think of a better fix than tracking the previous node in the recursive lookup of getJSDocCommentsAndTags, which is a little clunky.
Edit: This is no longer true; I just check for the original node, which should be good enough.
Fixes #24129