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.
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 property description #50269
Jsdoc property description #50269
Changes from all commits
0c51f47
3add1f9
c74367d
84e4611
8ba9d2c
6af103b
f404c80
50372d9
868ef27
ecfbf8b
cca8b27
b7011f9
bef80c2
38e27cc
c42189b
6440898
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Why are we creating a new symbol here? Given
every mention of
foo.bar
would have its own copy of the index symbol, which seems wrong.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 suggested it to handle filtering the symbol's declarations to only those matching the use site in order to handle when multiple index signatures exist (since all index signatures usually get tossed into the same __index symbol, rather than each having their own). Certainly, if the filtered list of applicable index signature declarations is identical to the full list, reusing the original symbol should be OK, and we could probably cache and reuse symbols when they're the same declaration subset across usages.
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.
New approach, as explained by @weswigham
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.
SymbolFlags.Signature
is also used for call and construct signatures. If tests are passing this might be fine, but I would want to understand why you don’t have to worry about call and construct signatures here.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 am actually not sure about this. The function returns ScriptElementKind.memberVariableElement ('property') for
SymbolFlags.Property
which is the other example I was looking at, I want to return ScriptElementKind.indexSignatureElement ('index') for SymbolFlags.Signature which is why I added this here, but I wasn't aware of SymbolFlags.Signature also being used for call and construct signatures.