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

Allow passing the project file as environment. #227

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

fredrikekre
Copy link
Member

No description provided.

pfitzseb
pfitzseb previously approved these changes Jul 16, 2021
Copy link
Member

@pfitzseb pfitzseb left a comment

Choose a reason for hiding this comment

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

Thanks!
Test failure is unrelated and should be fixed by #228.

@pfitzseb pfitzseb closed this Jul 16, 2021
@pfitzseb pfitzseb reopened this Jul 16, 2021
@davidanthoff davidanthoff added the enhancement New feature or request label Jul 17, 2021
@davidanthoff davidanthoff added this to the Next Patch milestone Jul 17, 2021
@davidanthoff
Copy link
Member

Why do we want to do that? I'm generally not a fan of making APIs behave a la "you can pass it either as a folder or file path, we'll just handle both cases", that seems to just add unnecessary complexity. And in this case we've used the directory path forever in the API, and the VS Code client won't pass anything else. Could other clients not just conform with that as well?

I do like the other aspects of the PR, certainly is a bit more elegant :)

@fredrikekre
Copy link
Member Author

It's just nice to be able to pass the output of Base.active_project() I guess?

It also seems that the full path is "more canonical" since otherwise the library has to guess what the user meant (although having both Project.toml and JuliaProject.toml is pretty uncommon).

@davidanthoff
Copy link
Member

I think if this is the canonical way, then we should change everything to make this the only way of specifying environments.

@davidanthoff davidanthoff modified the milestones: Next Patch, Backlog Jul 24, 2021
Copy link
Member

@davidanthoff davidanthoff left a comment

Choose a reason for hiding this comment

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

So I think if someone wants to change this throughout the LS and SymbolServer, then we could make that change, so that we just accept file names everywhere. It seems a fairly involved change, though, and at the moment I think we have such a backlog in terms of bugs that we probably should try to fix those first...

@fredrikekre
Copy link
Member Author

fredrikekre commented Oct 12, 2022

accept file names everywhere

This is the only place where it is not accepted to pass the file I believe. Everywhere else where the environment is used it is interpreted by Juila (which accepts either the directory or the file).

@davidanthoff davidanthoff removed this from the Backlog milestone Oct 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants