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

Create fake env when using a package dir as an env #498

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Conversation

ZacLN
Copy link
Contributor

@ZacLN ZacLN commented Jan 27, 2020

I've not tried this but feel it should work. When the target env dir is a package it modifies the Project.toml, adding the parent package + packages in extras.

@davidanthoff
Copy link
Member

Yeah, probably something like that... I'll also think a bit about this.

Should we do this for v0.14.1? I don't feel it fixes a regression, right? This never worked :)

@davidanthoff davidanthoff added this to the Next milestone Jan 27, 2020
@ZacLN
Copy link
Contributor Author

ZacLN commented Jan 27, 2020

Yeah, no rush. I just thought I'd put it down while I was thinking about it

@codecov
Copy link

codecov bot commented Jan 27, 2020

Codecov Report

Merging #498 into master will increase coverage by <.01%.
The diff coverage is 30%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #498      +/-   ##
==========================================
+ Coverage   28.77%   28.78%   +<.01%     
==========================================
  Files          22       22              
  Lines        1557     1567      +10     
==========================================
+ Hits          448      451       +3     
- Misses       1109     1116       +7
Impacted Files Coverage Δ
src/languageserverinstance.jl 33.75% <30%> (-0.54%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7d7944a...a66c1aa. Read the comment docs.

@davidanthoff
Copy link
Member

Ok, thought about this a bit. I have some basic questions first :)

Should we ever load anything that is defined in the current workspace folder via SymbolServer.jl? I had assumed that everything inside the current workspace folder (i.e. the thing open in VS Code) is actually handled by parsing files with CSTParser.jl, and then extracting the information from that? And that SymbolServer.jl is only used for things that are outside of the current workspace folder.

If we are loading stuff from the current workspace via SymbolServer.jl, how are we actually handling updates? That stuff gets edited all the time, and we can't rerun SymbolServer.jl after each key stroke, right?

In general we are trying to solve a problem for files outside the src folder in a package, right? So in particular in test and say folders like examples, i.e. in files that are not part of the package itself, but are using it.

For those files we seem to have two problems: 1) if they do using PackageThatIsEdited they won't find it (because that package is not part of the active env), and 2) if a file runtests.jl has a using Foo, but for is only in the extras section, right?

For 1), it seems to me that ideally we wouldn't modify at all what SymbolServer.jl does, but instead somehow expose the completion information that we already use when someone edits say src/PackageThatIsEdited.jl, but expose that here?

For 2), things get tricky, because in that situation the files actually really run in a different env than the active env. But that is true not just for the situation where we open the package Project.toml as the env, it is also true when the current working folder is deved in some global env. There are also similar situations with for example the docs folder. It can have a Project.toml, that is yet again distinct, and then the expectation would be that inside docs/make.jl that env is used to resolve names.

I think for 2) we probably need a more involved solution, that roughly looks like this: we assign a default active env to the root folder of the workspace, but we enable something where one can attach a different env to child folders as well. So essentially we would have multiple store things active at anyone point. One for the root, but then we might have assigned a different store to test and yet another one to doc and yet another one to examples. We would probably have to run SymbolServer three times in that case, once for each of these environments.

When we resolve names then in a file, we traverse the directory tree, until we find a folder that has an env attached to it and then use that store to resolve names there.

These env might sometimes be based on concrete env, but sometimes they will be based on constructed/temp env, for example in the case of the test folder.

We might need a call to sort this out?

@davidanthoff
Copy link
Member

Oh, and some more random complications: whenever we create a temp env, do we have to instantiate it? I'd hate that because it would start to download stuff... But I also don't see how we could get around that.

@ZacLN
Copy link
Contributor Author

ZacLN commented Jan 27, 2020

I'm just selectively picking out a few points:

Should we ever load anything that is defined in the current workspace folder via SymbolServer.jl?

This only acts on the path passed to the environment handling, so I imagine would only ever change if the user clicked the button at the bottom to change environments.

And that SymbolServer.jl is only used for things that are outside of the current workspace folder.

I think in this case the Project.toml has to be considered as its describing what to look for outside of the workspace folder.

In general we are trying to solve a problem for files outside the src folder in a package, right? So in particular in test and say folders like examples, i.e. in files that are not part of the package itself, but are using it.

Agreed, we could (when editing a package folder) determine which files are using the extras part of the Project and split out access to packages. This'd just need the language server side to read the Project file as well.

... but instead somehow expose the completion information that we already use when someone edits say src/PackageThatIsEdited.jl, but expose that here?

I don't understand this? And the rest seems reasonable.

A call would probably be useful to sort this out. I think the main thing is that I think this can be fixed without changing any SymServ behaviour (i think the code in this PR is all that would be needed) and that the rest could be handled by the language server

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

Successfully merging this pull request may close these issues.

None yet

2 participants