-
Notifications
You must be signed in to change notification settings - Fork 73
fix: isInWorkspace should work on closed files. #1004
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
Conversation
The initializeParams get sent the first time every language server gets started. When you open a new workspace it starts a new process and the client side and the language server side declare their features, send initialization params, etc If you add a workspace folder rather than open a new workspace, it should send a |
const normalizedChildPath = path.normalize(childPath) | ||
|
||
const relative = path.relative(normalizedParentPath, normalizedChildPath) | ||
return relative !== '' && !relative.startsWith('..') && !path.isAbsolute(relative) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically empty string would be OK, right? That just means it's the actual workspace folder itself.
assert.ok(isParentFolder('/foo', '/foo/bar/'), 'trailing slash in child') | ||
assert.ok(isParentFolder('/foo', '/foo/bar.txt'), 'files') | ||
assert.ok(isParentFolder('/foo', '/foo/bar/baz'), 'neseted directory') | ||
assert.ok(!isParentFolder('/foo', '/foo'), 'duplicates') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While this is correct for isParentFolder
, semantically inWorkspace
should return true in this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I was trying to write this utility such that it can easily re-used, but for the use-case here I agree that should work. I'll change it and update the tests.
Problem
Follow up: #969 (comment).
This utility function didn't do exactly what its expected to do. See the discussion linked above for more information.
Solution
Question:
Do the initializeParams update when the info changes? Ex. if I open a new workspace, are these updated? In my testing, the answer appears to be yes, because the language server reloads, but would like to confirm this somehow.answered below, thanks @jpinkney-aws.
License
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.