Skip to content

Support ~, ./ and named workspace folders in cwd #4687

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
Aug 9, 2023
Merged

Support ~, ./ and named workspace folders in cwd #4687

merged 8 commits into from
Aug 9, 2023

Conversation

andyleejordan
Copy link
Member

@andyleejordan andyleejordan commented Aug 5, 2023 •
edited
Loading

This makes them way less annoying. I'm ok taking a dependency on untildify as it's a very simple package, and the Python extension for VS Code also uses it. However, it must remain at v4.0.0, as the latest version, v5.0.0, is pure ESM and therefore cannot be loaded by VS Code.

https://github.com/sindresorhus/untildify/releases/tag/v5.0.0

Now also handles paths relative to a single workspace folder, or understands the name of a workspace folder in multi-root workspaces. Resolves #4557.

@andyleejordan andyleejordan added Issue-Enhancement A feature request (enhancement). Area-Configuration labels Aug 5, 2023
@andyleejordan andyleejordan requested a review from a team August 5, 2023 02:05
@andyleejordan andyleejordan force-pushed the andyleejordan/untildify branch from 09cfa4b to 3ef57f9 Compare August 5, 2023 02:07
@andyleejordan
Copy link
Member Author

andyleejordan commented Aug 5, 2023 •
edited
Loading

I definitely thought we had an issue (possibly closed) for this...it's come up at least, a number of times. @JustinGrote any idea where that went?

Base automatically changed from andyleejordan/better-finding to main August 8, 2023 13:30
@andyleejordan andyleejordan force-pushed the andyleejordan/untildify branch 2 times, most recently from c5a309c to 3bb69d7 Compare August 8, 2023 22:11
@andyleejordan andyleejordan requested a review from a team as a code owner August 8, 2023 22:11
@andyleejordan
Use untildify for ~ in cwd and additionalPowerShellExes
abb5ef6 
This makes them way less annoying. I'm ok taking a dependency on
untildify as it's a very simple package, and the Python extension for VS
Code also uses it. However, it must remain at v4.0.0, as the latest
version, v5.0.0, is pure ESM and therefore cannot be loaded by VS Code.

https://github.com/sindresorhus/untildify/releases/tag/v5.0.0
@andyleejordan
Add tests for validateCwdSetting
0094d35 
Those were overdue!
@andyleejordan
Stop using this.sessionSettings.cwd directly
aa6dd8e 
So we can remove weirdly placed `validateCwdSetting` calls and instead
use it exactly when required.
@andyleejordan
Support relative paths in cwd and add getChosenWorkspace
e3743c5 
This cleans up how we remember which workspace the user chose, and sets
us up to save that information not in the `cwd` setting. Refactors and
fixes `validateCwdSetting` to treat the empty string as undefined for
`cwd`.
@andyleejordan
Fix up settings tests and add test for cwd relative path support
b98320e
@andyleejordan andyleejordan force-pushed the andyleejordan/untildify branch from 3bb69d7 to e63bdc5 Compare August 8, 2023 22:13
@andyleejordan andyleejordan enabled auto-merge August 8, 2023 22:20
@JustinGrote
Copy link
Collaborator

I definitely thought we had an issue (possibly closed) for this...it's come up at least, a number of times. @JustinGrote any idea where that went?

Maybe you are thinking of #3827 which was closed?

@andyleejordan andyleejordan force-pushed the andyleejordan/untildify branch from e63bdc5 to c0594bd Compare August 9, 2023 05:05
@ghost ghost added the Issue-Bug A bug to squash. label Aug 9, 2023
@andyleejordan
Copy link
Member Author

Hm, that wasn't it @JustinGrote. Maybe it was just on Discord. Anyway, this resolves #4557 now since it no longer writes the cwd to the workspace settings. For multi-root workspaces, instead a name can be set (like 'vscode-powershell' for this repo) and then that workspace folder will be used. For a single-root workspace, a relative path will be resolved to its root.

SeeminglyScience
SeeminglyScience approved these changes Aug 9, 2023
View reviewed changes
Copy link
Collaborator

@SeeminglyScience SeeminglyScience left a comment

Choose a reason for hiding this comment

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

LGTM!

@andyleejordan andyleejordan added this pull request to the merge queue Aug 9, 2023
@JustinGrote
Copy link
Collaborator

I'll run this one through the paces once the VSIX is built, I have a couple multi root workspaces I can test against.

@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 9, 2023
@JustinGrote JustinGrote mentioned this pull request Aug 9, 2023
Closed
2 tasks
@andyleejordan
Copy link
Member Author

@JustinGrote awesome! Please do, I'll have a pre-release out soon. Main thing with multi-root workspaces is that this is only supporting putting the name of the workspace as the cwd. No relative paths to it or anything, just a choice. But it's not an absolute path any more!

@andyleejordan andyleejordan changed the title Use untildify for ~ in cwd and additionalPowerShellExes Support ~, ./ and named workspace folders in cwd Aug 9, 2023
@andyleejordan andyleejordan added this pull request to the merge queue Aug 9, 2023
@JustinGrote
Copy link
Collaborator

LGTM with multi workspace that I have, just a comment on the UX on the original issue.

@andyleejordan andyleejordan removed this pull request from the merge queue due to a manual request Aug 9, 2023
@andyleejordan
Add prompt to save multi-root workspace choice to cwd
fe8cc41 
So it's not just automatic, but can be done easily (and is an
appropriate value to sync with workspace settings).
@andyleejordan
Increase unit test timeout to 10 minutes
4832287 
And Mocha's debugger hook-up timeout to 30 seconds. Now unit tests can
be debugged easily, and the timeout is still appropriate enough for CI.
@andyleejordan andyleejordan added this pull request to the merge queue Aug 9, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 9, 2023
@andyleejordan andyleejordan added this pull request to the merge queue Aug 9, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 9, 2023
@andyleejordan andyleejordan merged commit 4f47aa4 into main Aug 9, 2023
@andyleejordan andyleejordan deleted the andyleejordan/untildify branch August 9, 2023 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JustinGrote JustinGrote JustinGrote left review comments

@SeeminglyScience SeeminglyScience SeeminglyScience approved these changes

Assignees

@JustinGrote JustinGrote

Labels
Area-Configuration Issue-Bug A bug to squash. Issue-Enhancement A feature request (enhancement).
Projects
Flying Fox
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow means to avoid adding an absolute Cwd path to the workspace settings
3 participants
@andyleejordan @JustinGrote @SeeminglyScience