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
Programmatic reference construction #774
Conversation
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.
Looks basically good to me.
Just one thing: isReference(reference) && reference.$refNode
looks a bit magic.
Can we instead have something like isParsedReference(reference)
to make difference clear?
@@ -60,7 +60,7 @@ export class DefaultReferencesProvider implements ReferencesProvider { | |||
if (targetAstNode) { | |||
const options = { includeDeclaration: params.context.includeDeclaration }; | |||
this.references.findReferences(targetAstNode, options).forEach(reference => { | |||
if (isReference(reference)) { | |||
if (isReference(reference) && reference.$refNode) { |
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.
Question: In which cases will a ReferenceDescription
be a proper Reference
. Is this legacy?
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.
Probably yes, I'll remove it.
After removing the code you found above, there was only one use site remaining, so I didn't add this for now. |
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 👍
What's the problem with !!
?
It's a pattern to convert something into a Boolean value, but double negation is always harder to understand than a direct statement ("I wouldn't say that noone has done it" vs. "Someone has done it"). |
Similar: |
See #773. The
$refNode
property is made optional so programmatic construction of (partial) ASTs with references is easier.