Skip to content
This repository was archived by the owner on Oct 8, 2025. It is now read-only.

Conversation

dvic
Copy link
Contributor

@dvic dvic commented Aug 11, 2023

TODO

  • make sure that they do not show up in Workspace Symbols
  • make sure dependencies don't show up in Find References
  • see if we need/can show the progress message when the database is finishing a "batch" of updates (message can say "indexing"). It can get bogged down and the go to def won't work until it's done inserting. Done in feat: progress messages for workspace indexing #179
  • tests
  • determine feasibility for elixir source files. they won't exist on disk, and since they aren't compiled, we won't have symbols for them, but there might be the possibility of opening the line of code in github in their web browser.
goto-def.mov

@mhanberg mhanberg mentioned this pull request Aug 11, 2023
4 tasks
@mhanberg
Copy link
Collaborator

tests pass for me locally

@mhanberg
Copy link
Collaborator

make sure dependencies don't show up in Find References

I don't think the existing tests handle this, so I wouldn't call it completed yet. same for workspace symbols

@mhanberg
Copy link
Collaborator

I think that this section of functionality might warrant it's own test module, as it will be testing workspace symbols, references, and definition.

that way you can restrict the proliferation of code that has to install a dependency to as few tests as possible, as literally downloading a dep during every test run is bound to be brittle and time consuming.

@mhanberg
Copy link
Collaborator

actually, it might make more sense to use a path dependency for these tests, and write those files in the same was as we do the normal tests.

actually running mix deps.get inside the tests seems like a red flag to me

@dvic
Copy link
Contributor Author

dvic commented Aug 12, 2023

actually, it might make more sense to use a path dependency for these tests, and write those files in the same was as we do the normal tests.

actually running mix deps.get inside the tests seems like a red flag to me

i think you’re right, but this got me thinking: don’t we want go to definition for path dependencies? does this include umbrella applications? how does this relate to workspaces? Let’s say I open a mono repo, it would be nice to have go to definition across projects.

@mhanberg
Copy link
Collaborator

Path deps should work the same as remote deps.

Umbrella apps should work already.

Monorepo project would most likely be a dep

@dvic
Copy link
Contributor Author

dvic commented Aug 12, 2023

@mhanberg what do you want to do actually for the workspace symbols: filter them in the query or avoid inserting them in the DB in te first place? If it's the latter: we do this in the sidecar then?

(this unpushed test is currently failing ^^)
image

@dvic
Copy link
Contributor Author

dvic commented Aug 12, 2023

I pushed the former, let me know if you want it differently :)

@dvic dvic marked this pull request as ready for review August 12, 2023 22:01
@mhanberg
Copy link
Collaborator

This PR is blocked on several things I am working on.

  1. Improved schema migration strategy
  2. Symbol indexing tracker
  3. Add new column to symbols table

@mhanberg
Copy link
Collaborator

You should be able to rebase now and work on this again.

There is a new "source" column on the symbols table that defaults to "user".

Then, when you create a separate tracer for compiling the dependencies, you can have that one insert the symbols with the source being set to "dep"

then when you are filtering on them for workspace symbols, you can say WHERE source = 'user'

@mhanberg
Copy link
Collaborator

CleanShot.2023-08-14.at.18.29.04.mp4

need to figure out how to handle this

@mhanberg
Copy link
Collaborator

CleanShot.2023-08-14.at.18.29.04.mp4

need to figure out how to handle this

I have a fix for this, just writing a test right now

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants