Skip to content
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

Add definitions and references for constructors #250

Merged
merged 6 commits into from
Apr 26, 2023
Merged

Conversation

SuperAuguste
Copy link
Contributor

@SuperAuguste SuperAuguste commented Apr 24, 2023

Closes #249

This replicates VSCode's gotodef.

Test plan

Tested locally with a snapshot, still need to test edge cases + implement and test inherited constructors.

@SuperAuguste SuperAuguste marked this pull request as draft April 24, 2023 21:34
Copy link
Member

@olafurpg olafurpg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Impressive first PR @SuperAuguste! This is definitely a tricky issue to fix and I see you've already become familiar with the tsc APIs 😄

It's mostly just me who has worked in this codebase so far and there are many undocumented conventions and assumptions. Don't hesitate to push back if some of these comments are too nitpicky, I prefer to have more relaxed conventions if it makes it easier for more people to contribute (rather than strictly enforcing some undocumented style making it a drudge to contribute).

src/FileIndexer.ts Outdated Show resolved Hide resolved
src/main.test.ts Outdated Show resolved Hide resolved
src/FileIndexer.ts Outdated Show resolved Hide resolved
snapshots/input/syntax/src/constructor.ts Outdated Show resolved Hide resolved
src/FileIndexer.ts Outdated Show resolved Hide resolved
src/FileIndexer.ts Outdated Show resolved Hide resolved
src/FileIndexer.ts Outdated Show resolved Hide resolved
@SuperAuguste SuperAuguste force-pushed the olafurpg/constructors branch from 6003a45 to f9212ac Compare April 25, 2023 14:31
@SuperAuguste SuperAuguste force-pushed the olafurpg/constructors branch from f9212ac to c60d510 Compare April 25, 2023 14:36
@SuperAuguste SuperAuguste requested a review from olafurpg April 25, 2023 14:37
@SuperAuguste SuperAuguste force-pushed the olafurpg/constructors branch from 808dfd5 to caa792d Compare April 25, 2023 14:45
@SuperAuguste SuperAuguste marked this pull request as ready for review April 25, 2023 14:48
@SuperAuguste
Copy link
Contributor Author

SuperAuguste commented Apr 25, 2023

CI failing due to different TypeScript versions :/

I'm kind of lost here, not sure why they're different to be frank.

Copy link
Member

@olafurpg olafurpg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is really shaping up! Only blocking comments are

  • Add back your constructor detection logic, but with a cache
  • Undo changes in yarn.lock, I suspect this might fix the failing CI

snapshots/package.json Outdated Show resolved Hide resolved
src/FileIndexer.ts Outdated Show resolved Hide resolved
@SuperAuguste
Copy link
Contributor Author

Undo changes in yarn.lock, I suspect this might fix the failing CI

The current package.json seems to be invalid due to a peer resolution issue (I ran yarn install --immutable --immutable-cache to not modify the yarn.lock) so my install won't go through. How do you install the deps for this repo without modifying the package.json/lockfile? :)

@SuperAuguste SuperAuguste force-pushed the olafurpg/constructors branch 2 times, most recently from 3c681b8 to 035d2f6 Compare April 25, 2023 18:57
@SuperAuguste
Copy link
Contributor Author

Figured out the issue - the yarn version used here is a few years behind it seems, so I installed v1.22.19 and everything worked out!

@SuperAuguste SuperAuguste force-pushed the olafurpg/constructors branch 2 times, most recently from 595c90b to e5c0992 Compare April 25, 2023 19:02
@SuperAuguste SuperAuguste requested a review from olafurpg April 25, 2023 19:03
Copy link
Member

@olafurpg olafurpg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Glorious 😍 Thank you @SuperAuguste!

@@ -1,5 +1,7 @@
# Developing scip-typescript

Please note that the yarn version used by CI is `v1.22.19` - you should use this version as well to prevent lockfile conflicts.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Followup #251

export function useNoConstructor() {
// ^^^^^^^^^^^^^^^^ definition syntax 1.0.0 src/`constructor.ts`/useNoConstructor().
// documentation ```ts\nfunction useNoConstructor(): NoConstructor\n```
return new NoConstructor()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! 🤩

const declaration = sym.declarations?.[0]
if (!declaration) {
return undefined
}
const signatureDeclaration: ts.SignatureDeclaration | undefined =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hadn't realized this was already inside a function 🤦🏻 The new code looks good though 👍🏻

@olafurpg olafurpg merged commit 82c3710 into main Apr 26, 2023
@olafurpg olafurpg deleted the olafurpg/constructors branch April 26, 2023 10:29
@olafurpg
Copy link
Member

. How do you install the deps for this repo without modifying the package.json/lockfile? :)

I normally run yarn without a subcommand

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Emit occurrences for class constructors and their usage sites
2 participants