-
Notifications
You must be signed in to change notification settings - Fork 21
Generate correct scipSymbol
for namespace imports
#195
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
Conversation
One thing I like to sometimes do here is first commit the test (and run update-snapshots -- so that tests are still passing) and then commit the fix with the updated snapshot. The commits are squashed when merging anyways, but the PR becomes slightly nicer to read (this commit is the fix, and here is how the snapshot output changed because of it). The flip side is that is becomes a bit more finicky if you want to go back and edit the test later. I've sometimes done this for PRs in scip-ruby this way; it also makes it easier to go back on look at older PRs even if it was only self-reviewed. For example:
I haven't seen other people do this though, so I'm not going to ask that you do the same, but just mentioning it as one option. 😅 |
252c6d2
to
e62facf
Compare
@varungandhi-src, I like your approach! Here's the line with the incorrectly generated symbol. And here's the update that fixes it. |
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.
This looks great. Thank you much @valerybugakov!
// ^^^^^^^^^^ reference typescript 4.8.4 lib/`typescript.d.ts`/ts/SyntaxKind# | ||
// ^^^^^^^^^ reference typescript 4.8.4 lib/`typescript.d.ts`/ts/SyntaxKind#ArrayType. | ||
namespace.a.value | ||
// ^^^^^^^^^ reference local 0 |
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.
Nice find 👍🏻
@varungandhi-src I regularly do this myself. It's a very helpful technique to verify that your diff actually fixes the issue. |
Without changes introduced in this PR
local 0
SCIP symbol is generated for thets
import.Test plan
See the updated test snapshot. Comment out the
FileIndexer
changes to reproduce the previous behavior.