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

Fix get_project_path for multi-directory workspaces #472

Merged
merged 4 commits into from
Dec 8, 2018
Merged

Fix get_project_path for multi-directory workspaces #472

merged 4 commits into from
Dec 8, 2018

Conversation

scriptis
Copy link
Contributor

@scriptis scriptis commented Dec 1, 2018

Fixes a logic error that causes get_project_path to always return the first directory in the current workspace.

@scriptis scriptis changed the title Fix #471 Fix get_project_path for multi-directory workspaces Dec 1, 2018
@coveralls
Copy link

coveralls commented Dec 1, 2018

Coverage Status

Coverage increased (+0.7%) to 28.69% when pulling 4844d70 on scriptis:master into 5f9b914 on tomv564:master.

@scriptis
Copy link
Contributor Author

scriptis commented Dec 1, 2018

While this does fix the CWD of spawned language servers, LSP still seems to stop functioning entirely when working in a non-primary directory in a workspace... I'm going to leave this open for the moment, but keep poking around to see if I can fix anything else.

To see the problem first-hand:

  • Install RLS
  • Load up two folders into one workspace (in my case, LSP and ripgrep)
  • In the latter folder, RLS loads (with this PR), but all of LSP's features are disabled, broken, or missing

The problem itself seems to be intermittent, and sometimes resolves itself when you restart Sublime, add/remove projects from your workspace, etc. This PR fixes things enough of the time to be worth merging, so I'll probably open a different PR sometime later this week if I can find the root of the problem.

plugin/core/workspace.py Outdated Show resolved Hide resolved
@rwols rwols added the bug label Dec 2, 2018
@rwols rwols added this to the Release 0.8 milestone Dec 2, 2018
@rwols
Copy link
Member

rwols commented Dec 8, 2018

What's the progress on this?

@scriptis
Copy link
Contributor Author

scriptis commented Dec 8, 2018

Haven't had time to work on this the past week, should have it sorted today. Sorry about the wait.

@scriptis
Copy link
Contributor Author

scriptis commented Dec 8, 2018

Look good? @rwols

Copy link
Member

@rwols rwols left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for your work!

@rwols rwols merged commit b8109b1 into sublimelsp:master Dec 8, 2018
@tomv564
Copy link
Contributor

tomv564 commented Dec 10, 2018

Thanks for the contribution, but I will revert this change now in favour of a full solution.

The existing behaviour was not a "logic error", the function even documented that it would return the first folder. (Also see the note in the troubleshooting section and issue #33 linked from it)

I'm sure we can make multi-folder projects work reliably, but if we need to look closer at the usages of get_project_path - what further changes are needed so get_project_path is always correct?

tomv564 added a commit that referenced this pull request Dec 10, 2018
tomv564 added a commit that referenced this pull request Dec 10, 2018
* Revert "Fix: get_project_path for multi-directory workspaces (#472)"

This reverts commit b8109b1.
@tomv564
Copy link
Contributor

tomv564 commented Dec 10, 2018

LSP should probably start communicating the current limitation by warning users and refusing server startup when files under the 2nd folder are opened.

@scriptis
Copy link
Contributor Author

scriptis commented Dec 10, 2018

@tomv564: I don't follow your reasoning in reverting this. For workspaces with more than one active project, this evaluates to true, always returning the first directory of the workspace, even though the function is documented to return the parent folder of the active view. As far as I can tell, this PR made the function conform to the previous documentation, not stray from it?

Further, as far as I am aware, this PR fixes the issues regarding LSP behavior in a multi-directory workspace. This was, after all, the entire point...

@tomv564
Copy link
Contributor

tomv564 commented Dec 11, 2018

The parent folder of the active view part of the documentation/function is intended as a fallback, it probably should have been worded as such including pointing out it is a filesystem directory it wants to return, not a sublime folder.

But perhaps this is a good time to look at supporting multi-directory workspaces:

Say you have a window with two python projects:

/project1
/project2

Server sessions are shared per window

  • You open /project1/main.py - the python language server starts with rootUri file:///project1
  • You then open /project2/main.py - the window already has a python session, so it re-uses that.
  • LSP sends didOpen project2/main.py on a session based on project1.

This is fixable with the cost of looking up the session by window root folder.

get_project_path uses window.active_view()

  • Request references in /project1/main.py
  • You switch active view to /project2/main.py
  • References response arrives, calls get_project_path and receives /project2 instead of /project1, breaking the relative path lookup

Support configuration per folder?

A follow-up consideration: If users can have different sessions for /project1 and /project2 in the same window, should they not be able to configure the language server differently for each folder?

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

Successfully merging this pull request may close these issues.

4 participants