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

feat(definition): go to dependency #171

Merged
merged 10 commits into from
Aug 15, 2023
Merged

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

test/next_ls_test.exs Outdated Show resolved Hide resolved
test/support/utils.ex Outdated Show resolved Hide resolved
test/support/utils.ex Outdated Show resolved Hide resolved
@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 :)

lib/next_ls/db.ex Outdated Show resolved Hide resolved
lib/next_ls.ex Outdated Show resolved Hide resolved
@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

@mhanberg mhanberg enabled auto-merge (squash) August 15, 2023 02:43
@mhanberg mhanberg merged commit ddd28de into elixir-tools:main Aug 15, 2023
3 checks passed
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.

2 participants