Skip to content
This repository has been archived by the owner on Apr 4, 2023. It is now read-only.

Deprecate current.project.path variable #585

Closed
wants to merge 1 commit into from
Closed

Deprecate current.project.path variable #585

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 5 additions & 31 deletions plugins/task-plugin/src/variable/project-path-variable-resolver.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,17 +9,17 @@
**********************************************************************/

import { injectable } from 'inversify';
import Uri from 'vscode-uri';
import * as che from '@eclipse-che/plugin';
import * as theia from '@theia/plugin';
import * as startPoint from '../task-plugin-backend';

const VARIABLE_NAME = 'current.project.path';
const SELECTED_CONTEXT_COMMAND = 'theia.plugin.workspace.selectedContext';
const PROJECTS_ROOT_VARIABLE = 'CHE_PROJECTS_ROOT';
const ERROR_MESSAGE_TEMPLATE = 'Can not resolve \'current.project.path\' variable.';
/**
* Contributes the variable for getting path for current project as a relative path to the first directory under the root workspace.
*
* @deprecated will be removed soon. Use ${workspaceFolder} macro instead.
Copy link
Member

@RomanNikitenko RomanNikitenko Dec 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I think it's not enough just replace ${current.project.path} by ${workspaceFolder}. Instead user should use ${workspaceFolder}/theia for example

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There will be created PR into docs that describes that fact, that this macro is deprecated and has to be replaced

*/
@injectable()
export class ProjectPathVariableResolver {
Expand All @@ -38,33 +38,7 @@ export class ProjectPathVariableResolver {
return this.onError('Projects root is not set');
}

const selections = await theia.commands.executeCommand<Uri[]>(SELECTED_CONTEXT_COMMAND);
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should not remove this logic if at the moment it's just deprecated?
For example, user starts a workspace from devfile with two defined projects: theia and che-theia.
By default user has /projects as workspace folder.
According to these changes ${current.project.path} returns /projects, not /projects/theia for example.
Isn't it?
Please correct me if I'm wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This command relies on the current selection. When workspace opens at first time, the selection is not initialized and we're getting error message ... bad substitution .... It will return /project only when user opened explorer part or some file in editor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

selections in case of opening workspace at first time will be undefined. So it useless and calling ${workspaceFolder} will return the same result if editor or explorer is opened.

Copy link
Member

Choose a reason for hiding this comment

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

I understand that there are bugs related to using this variable if user has incorrect selection, that's why we decided to use ${workspaceFolder} instead of ${current.project.path}.
But if user or our samples rely on ${current.project.path}, these places will be broken after your changes!
isn't is?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Our samples don't use this macro. If user uses this macro in own devfile, it will broken anyway and command won't execute. There will be created another PR into docs that describes, that fact, that this macro is deprecated and has to be replaced with another one. So, yes, this changes proposal changes behavior of the flow for this macro.

Copy link
Member

Choose a reason for hiding this comment

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

steps to reproduce:

  • start a workspace from a devfile, get project structure like:
    pr_structure
  • run the task:
{
                "type": "che",
                "label": "test2",
                "command": "sleep 3 && echo ${workspaceFolder}",
                "target": {
                    "workingDir": "/projects"
                }
            },

output: /projects

  • select che-theia project and run the task:
{
                "type": "che",
                "label": "test2",
                "command": "sleep 3 && echo ${current.project.path}",
                "target": {
                    "workingDir": "/projects"
                }
            }

output: /projects/che-theia

if (!selections || selections.length < 1) {
return this.onError('Please select a project.');
}

const selection = selections[0];
const selectionPath = selection.path;
const workspaceFolder = theia.workspace.getWorkspaceFolder(theia.Uri.file(selectionPath));
if (!workspaceFolder) {
return this.onError('Selection doesn\'t match any workspace folder.');
}

const workspaceFolderPath = workspaceFolder.uri.path;
if (workspaceFolderPath === this.projectsRoot) {
const splittedSelectionUri = selectionPath.substring(workspaceFolderPath.length).split('/');
const project = splittedSelectionUri.shift() || splittedSelectionUri.shift();

this.isResolved = true;
return `${this.projectsRoot}/${project}`;
}

if (workspaceFolderPath.startsWith(this.projectsRoot)) {
this.isResolved = true;
return workspaceFolderPath;
}

return this.onError('The selection isn\'t under the current workspace root folder.');
return this.projectsRoot;
}

private createVariable(): che.Variable {
Expand All @@ -76,10 +50,10 @@ export class ProjectPathVariableResolver {
};
}

private onError(error?: string) {
private onError(error: string) {
this.isResolved = false;

const errorMessage = error ? `${ERROR_MESSAGE_TEMPLATE} ${error}` : ERROR_MESSAGE_TEMPLATE;
const errorMessage = `${ERROR_MESSAGE_TEMPLATE} ${error}`;
theia.window.showErrorMessage(errorMessage);
return Promise.reject(errorMessage);
}
Expand Down