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

[vscode] support for 'pathSeparator' variable substitution #9054

Merged
merged 1 commit into from
Mar 1, 2021

Conversation

alex-gilin
Copy link
Contributor

@alex-gilin alex-gilin commented Feb 11, 2021

Signed-off-by: Alexander Gilin alexander.gilin@sap.com

What it does

vscode alignment: allow using the predefined variable ${pathSeparator} (the character used by the operating system to separate components in file paths) in Debugging and Task configuration files

How to test

Variable substitution is supported inside key and value strings in launch.json and tasks.json files.
Expecting result: ${pathSeparator} - / on macOS or linux, \ on Windows

  • create simple "Hello World" TypeScript project like follows:
    create an empty folder "myProject", generate a tsconfig.json file and open that folder in theia:
    mkdir myProject
    cd myProject
    tsc --init
    Now create a 'HelloWorld.ts' file with the following content:
    function sayHello(name: string): void {
    console.log('Hello ' + name + '!');
    }

    sayHello('Dave');

  • compile the code : tsc -p .

  • open debug view -> Add configuration -> Node.js Launch Program
    In the created configuration change a path to the program like next:
    "program": "${workspaceFolder}${pathSeparator}helloworld.js"

  • run this configuration and verify the program path is resolved correctly.

Review checklist

Reminder for reviewers

@amiramw
Copy link
Member

amiramw commented Feb 11, 2021

@alex-gilin please follow open PR template

@alex-gilin alex-gilin changed the title [vscode] support for 'pathSeparator' variable substitution, [vscode] support for 'pathSeparator' variable substitution Feb 11, 2021
@@ -18,6 +18,7 @@ import { injectable, inject } from 'inversify';
import { VariableContribution, VariableRegistry } from './variable';
import { EnvVariablesServer } from '@theia/core/lib/common/env-variables';
import { CommandService } from '@theia/core/lib/common/command';
import { isWindows } from '@theia/core/lib/common/os';
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity, what's the intention here? So if the backend is a Linux machine and the frontend is running on a Windows node, what do you want to see: \\ or /?

If you need the backend OS, you have to use:

async getBackendOS(): Promise<OS.Type> {
return OS.type();
}

If you were aware of all these 👆 , just ignore my comment. 😊

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kittaakos
I agree, good point. Updated.

@kittaakos
Copy link
Contributor

I would do like this, to make sure the backend OS is resolved by the time you register the variables. I did not debug into your code, but your isWindowsBackend variable could give a false-negative result because it is not yet resolved by the time you're doing the variable registration.

diff --git a/packages/variable-resolver/src/browser/common-variable-contribution.ts b/packages/variable-resolver/src/browser/common-variable-contribution.ts
index 318f87d3c..b755f9a81 100644
--- a/packages/variable-resolver/src/browser/common-variable-contribution.ts
+++ b/packages/variable-resolver/src/browser/common-variable-contribution.ts
@@ -24,6 +24,8 @@ import { VariableInput } from './variable-input';
 import { QuickInputService } from '@theia/core/lib/browser/quick-open/quick-input-service';
 import { QuickPickService, QuickPickItem } from '@theia/core/lib/common/quick-pick-service';
 import { MaybeArray, RecursivePartial } from '@theia/core/lib/common/types';
+import { ApplicationServer } from '@theia/core/lib/common/application-protocol';
+import { OS } from '@theia/core/lib/common/os';
 
 @injectable()
 export class CommonVariableContribution implements VariableContribution {
@@ -46,8 +48,18 @@ export class CommonVariableContribution implements VariableContribution {
     @inject(QuickPickService)
     protected readonly quickPickService: QuickPickService;
 
+    @inject(ApplicationServer)
+    protected readonly appServer: ApplicationServer;
+
     async registerVariables(variables: VariableRegistry): Promise<void> {
-        const execPath = await this.env.getExecPath();
+        const [execPath, backendOS] = await Promise.all([
+            this.env.getExecPath(),
+            this.appServer.getBackendOS()
+        ]);
+        variables.registerVariable({
+            name: 'pathSeparator',
+            resolve: () => backendOS === OS.Type.Windows ? '\\' : '/'
+        });
         variables.registerVariable({
             name: 'execPath',
             resolve: () => execPath

@alex-gilin
Copy link
Contributor Author

@kittaakos ,
in my approach I relied on 'postConstruct' invocation order. But anyhow, your proposal better :)

where ${pathSeparator} - / on macOS or linux, \\ on Windows

Signed-off-by: Alex Gilin <alexander.gilin@sap.com>
Copy link
Contributor

@kittaakos kittaakos left a comment

Choose a reason for hiding this comment

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

👍 Thank you for the patch, @alex-gilin

@amiramw amiramw merged commit f8bcc19 into eclipse-theia:master Mar 1, 2021
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.

3 participants