Introduce the Variable Resolver Extension #1581

azatsarynnyy · 2018-03-26T15:12:39Z

Signed-off-by: Artem Zatsarynnyi [email protected]

PR closes #1561

This PR introduces the variable-resolver extension which provides support of variable substitution mechanism inside strings. Extension adds:

a contribution point to provide a hook that allows any extensions to contribute its own variables e.g.:

export class EditorVariableContribution implements VariableContribution {
    registerVariables(variables: VariableRegistry): void {
        variables.registerVariable({
            name: 'lineNumber',
            description: 'The current line number in the current file',
            resolve: () => {
                const currentEditor = ...
                return Promise.resolve(`${currentEditor.cursor.line + 1}`);
            }
        });
    }
}

VariableResolverService allows to resolve all known variables in a string e.g.:

const resolved = await service.resolve('file: ${file}; line: ${lineNumber}');
expect(resolved).is.equal('file: package.json; line: 6');

VariableQuickOpenService allows to get list of all registered variables e.g.:

Note, this PR doesn't contribute any variables. I'm going to provide a couple of separate PRs a bit later to add support for variables in tasks.json and add the VSCode's predefined variables to add a little bit more compatibility with VSCode Tasks.

kittaakos · 2018-03-26T17:22:48Z

packages/variable-resolver/src/browser/variable.spec.ts

+import { ILogger } from '@theia/core/lib/common';
+import { MockLogger } from '@theia/core/lib/common/test/mock-logger';
+import { Variable, VariableRegistry } from './variable';
+import { Disposable } from 'vscode-jsonrpc';


I do not know whether it was intentional or not, but we usually user @theia/core/common/disposable. I guess the tooling did it.

Yes, it was VSCode's work ))
Fixed import.

kittaakos · 2018-03-26T17:24:58Z

packages/variable-resolver/src/browser/variable.ts

+    /**
+     * A unique name of this variable.
+     */
+    name: string;


Did you omit the readonly on purpose or not? If by accident, please add it. Thank you!

Just overlooked. Annotated both properties as readonly.

kittaakos · 2018-03-26T17:27:41Z

packages/variable-resolver/src/browser/variable.ts

+    }
+
+    /**
+     * Get a variable for the given name or 'undefined' if none.


Could you please use ` instead of '? The documentation looks better in VSCode. What do you think?

Absolutely agree. Changed

kittaakos · 2018-03-26T17:31:16Z

packages/variable-resolver/src/browser/variable-quick-open-service.ts

+    }
+
+    run(mode: QuickOpenMode): boolean {
+        if (mode !== QuickOpenMode.OPEN) {


Then, it is always false, isn't it?

Sure. Changed.

kittaakos · 2018-03-26T17:35:58Z

packages/variable-resolver/src/browser/variable-resolver-frontend-contribution.ts

+
+export const LIST_VARIABLES: Command = {
+    id: 'variable.list',
+    label: 'Variable: list all'


Could we have Variable: List All instead? At least, that was the convention for instance in the Git extension so far.

kittaakos

Super nice, clean code!

epatpol · 2018-03-26T19:15:41Z

Looks very good! Maybe add a short example in the README on how you can use the substitutions i.e implement a contribution and inject the service?

akosyakov · 2018-03-27T06:27:37Z

packages/variable-resolver/src/browser/variable-quick-open-service.ts

@@ -0,0 +1,68 @@
+/*
+ * Copyright (C) 2018 Red Hat, Inc.


I think the copyright should be under Apache 2 license:

/* * Copyright (C) 2018 Read Hat, Inc. and others. * * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. * You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0 */

cc @svenefftinge

I've updated all files

akosyakov · 2018-03-27T06:38:47Z

packages/variable-resolver/src/browser/variable.ts

+     * 'undefined' if variable cannot be resolved.
+     * Never reject.
+     */
+    resolve(): Promise<string | undefined>;


You can use MaybePromise here, that implementors does not deal with promises. You can resolve a maybe promise with await syntax or Promise.resolve.

Yeah, with MaybePromise it looks more flexible. I've updated it and also slightly refactored the VariableResolverService.
Thanks for the advice.

akosyakov · 2018-03-27T06:51:46Z

packages/variable-resolver/src/browser/variable-quick-open-service.ts

+        const quickOpenItems = this.variableRegistry.getVariables().map(
+            v => new VariableQuickOpenItem(v.name, v.description)
+        );
+        this.items = [...quickOpenItems];


this.items = quickOpenItems ? map fn creates a new array

Sure ) Changed

Signed-off-by: Artem Zatsarynnyi <[email protected]>

azatsarynnyy · 2018-03-27T13:48:53Z

@kittaakos @epatpol @akosyakov
Guys, thanks for the reviews. I've addressed all your comments and rebased the branch.
@epatpol I've updated the README with the examples of using.

epatpol · 2018-03-27T14:06:28Z

Yes I didn't expect a readme that extensive, very well done thank you!

azatsarynnyy · 2018-03-28T08:14:32Z

Guys, could someone give me the approval to merge this PR if it looks good for you?

kittaakos · 2018-03-28T08:18:32Z

give me the approval

The PR looks great. @akosyakov, do you have any additional remarks on this?

azatsarynnyy mentioned this pull request Mar 26, 2018

Support variable substitution inside strings #1561

Closed

kittaakos reviewed Mar 26, 2018

View reviewed changes

akosyakov reviewed Mar 27, 2018

View reviewed changes

Introduce the Variable Resolver Extension

2426b87

Signed-off-by: Artem Zatsarynnyi <[email protected]>

azatsarynnyy force-pushed the 1561 branch from b1e728a to 2426b87 Compare March 27, 2018 13:26

akosyakov approved these changes Mar 28, 2018

View reviewed changes

azatsarynnyy merged commit ea79ea1 into master Mar 28, 2018

azatsarynnyy deleted the 1561 branch March 28, 2018 09:24

This was referenced Mar 28, 2018

Support variables in Tasks #1596

Closed

Support commands macro in Theia tasks eclipse-che/che#8974

Closed

slemeur mentioned this pull request Apr 11, 2018

Commands support in next gen Che eclipse-che/che#8382

Closed

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce the Variable Resolver Extension #1581

Introduce the Variable Resolver Extension #1581

azatsarynnyy commented Mar 26, 2018

kittaakos Mar 26, 2018

azatsarynnyy Mar 27, 2018

kittaakos Mar 26, 2018

azatsarynnyy Mar 27, 2018

kittaakos Mar 26, 2018

azatsarynnyy Mar 27, 2018

kittaakos Mar 26, 2018

azatsarynnyy Mar 27, 2018

kittaakos Mar 26, 2018

azatsarynnyy Mar 27, 2018

kittaakos left a comment

epatpol commented Mar 26, 2018

akosyakov Mar 27, 2018

azatsarynnyy Mar 27, 2018

akosyakov Mar 27, 2018

azatsarynnyy Mar 27, 2018

akosyakov Mar 27, 2018

azatsarynnyy Mar 27, 2018

azatsarynnyy commented Mar 27, 2018

epatpol commented Mar 27, 2018

azatsarynnyy commented Mar 28, 2018

kittaakos commented Mar 28, 2018

Introduce the Variable Resolver Extension #1581

Introduce the Variable Resolver Extension #1581

Conversation

azatsarynnyy commented Mar 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kittaakos left a comment

Choose a reason for hiding this comment

epatpol commented Mar 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azatsarynnyy commented Mar 27, 2018

epatpol commented Mar 27, 2018

azatsarynnyy commented Mar 28, 2018

kittaakos commented Mar 28, 2018