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

support ~ completions, wrap file/folder completion item in quotes if it contains spaces #234600

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

meganrogge
Copy link
Contributor

@meganrogge meganrogge commented Nov 25, 2024

fixes #234352

also pulls more test variables into constants and makes two testSpec properties optional to reduce duplication

@meganrogge meganrogge self-assigned this Nov 25, 2024
@vs-code-engineering vs-code-engineering bot added this to the November 2024 milestone Nov 25, 2024
@meganrogge meganrogge enabled auto-merge (squash) November 25, 2024 20:54
@@ -201,10 +201,14 @@ export class TerminalCompletionService extends Disposable implements ITerminalCo

const parentDirPath = cwd.fsPath.split(resourceRequestConfig.pathSeparator).slice(0, -1).join(resourceRequestConfig.pathSeparator);
const parentCwd = URI.from({ scheme: cwd.scheme, path: parentDirPath });
const homeDir = cwd.with({ path: '' });
Copy link
Contributor Author

Choose a reason for hiding this comment

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

hm, just realized this isn't quite right. we need to get the $HOME var, but it's not available on process here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess we have to wait until env is provided via shell integration?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

going to try terminal shell env service

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep that works

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that causes a layering issue, which is a bummer bc it works so nicely 😢

Copy link
Member

Choose a reason for hiding this comment

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

@meganrogge I think IShellEnvironmentService or similar* is what you want. To do this you'll need to create a base abstract TerminalCompletionService, then one for browser (that handles when HOME isn't available) and one for electron-sandbox (when HOME is available). Then pull via dependency injection and pass as args into super(). Look at ITerminalProfileResolverService for an example of how to do this.

* I notice it's in electron-sandbox though so you'll also need to pull the environment in the case of remotes. Maybe hold off on that since we'll be moving on to the upcoming shell environment API stuff which will have it on a new capability.

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

Successfully merging this pull request may close these issues.

Terminal suggest: Have ~ as completion for directories
2 participants