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 clojure-lsp regressions from #2020 #2046
Conversation
When the `"when-workspace-opened-use-workspace-root"` setting is enabled the clojure-lsp is started in _all_ workspace roots open instead of only valid clojure project roots. This commit reworks this behaviour to first check if the workspace root is a valid Clojure project before attempting to auto-start the lsp server. Addresses BetterThanTomorrow#2041
This resolves a regression from BetterThanTomorrow#2020 which prevents certain vscode operations such as organise-imports and add-missing-import from functioning correctly. This was a result of disabling the automatic command registrations being performed by the lsp client. Originally it was assumed that these commands were unused, however they are used internally by vscode to handle their native organise-imports and related operations. This is now fixed by manually registering the missing commands with the added functionality of multi-client support. Addresses BetterThanTomorrow#2041 Fixes BetterThanTomorrow#2040
The clojure-lsp refactor project in BetterThanTomorrow#2020 reworked the way the project picker menus work, adding additional information on a second line indicating which files contributed to a folder being picked as a valid clojure project. This turned out significant hurt the UX experience in workspaces with many clojure projects. This commit removes the additional information from the picker menus, shrinking the list items to a single line. Some other smaller changes included: - Projects are now grouped by their workspace root which should help make it easier to sort through the list visually. - Project folders are now sorted within their groups to make the list more deterministic. - Project folders show the path relative to the workspace root instead of the absolute path. This should help make scanning the list easier by removing unnecessary/duplicate information. Addresses BetterThanTomorrow#2041 Closes BetterThanTomorrow#2043
Thanks! To my testing this works as you advertise. I think that tucking the project file to the end of the path can get a bit confusing. E.g. here the shadow-w-backend project has several project files, |
Oops this is a bug :) will fix. Resolved in 13d539f |
This changes how it behaves today when opening a folder w/o a Clojure project at the root. Not sure we should necessarily keep that behaviour (ought to be pretty uncommon, and clojure-lsp help gets pretty limited). WDYT, @julienvincent @bpringe @ericdallo? |
I also expect it would be pretty uncommon. As a fallback users can always explicitly start the clojure-lsp server in the root if they want as well. |
@PEZ Are we concerned about this/anything else? Anything outstanding before this can be ready for merge? |
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.
Just left a suggestion and question.
The changes look good to me overall, though I haven't tested them. If @PEZ thinks it's good to go when he has a chance to look/test, then let's merge 🚀.
Thanks for the fixes!
Co-authored-by: Brandon Ringe <12722744+bpringe@users.noreply.github.com>
I think this is good to go. Will merge in a jiffy. Thanks again. I love working with you, @julienvincent! A thing we can follow up with in a PR is to add a reason to the pre-selected project. In the jack-in/connect project menu, it is pulled out of, and breaks, the sort order. For good reasons, but the user isn't told those. In the screenshot the user is told a ”reason” for the first entry, it's the Workspace root. The selected item could say something like Closest Project. Yeah, that's the reason why it is pre-selected, not why it is sorted where it is, but anyway. I like that the workspace root is sorted to the top, btw. It makes sense and chimes with the rest of the ordering. |
A regression introduced in BetterThanTomorrow#2020 and BetterThanTomorrow#2046 prevented the clojure-lsp and repl jack-in operations from working correctly on Windows hosts. This fixes the way we handle Uri's to make the unix and windows behaviours consistent. Fixes BetterThanTomorrow#2054
This is a collection of fixes which should resolve the regressions and issues reported as a result of #2020.
"when-workspace-opened-use-workspace-root"
setting the clojure-lsp server is only started in valid clojure workspaces.Organise Imports
continue to work. This was an unforeseen side-effect of disabling the dynamic lsp command registrations the client was performing. This default behaviour is still disabled, but now we manually register the commands (once) and augment them with support for selecting the correct lsp client to forward the request to.Project Menus
There were a few changes made to the project menus:
details
on why the project was considered has been removed.When sorting projects the behaviour is as follows:
Checklist
dev
branch. (Or have specific reasons to target some other branch.)published
. (Sorry for the nagging.)[Unreleased]
entry inCHANGELOG.md
, linking the issue(s) that the PR is addressing.ci/circleci: build
test.npm run prettier-format
)npm run eslint
before creating your PR, or runnpm run eslint-watch
to eslint as you go).Ping @PEZ, @bpringe, @corasaurus-hex, @Cyrik