fix: Install generator to location start with tild ~ #535

tomer-epstein · 2021-03-31T13:20:49Z

No description provided.

slavik-lvovsky

I see several problems:

~ comes from linux/unix, does not exist in windows
~~ would give wrong result

tomer-epstein · 2021-03-31T15:52:05Z

I see several problems:

~ comes from linux/unix, does not exist in windows

~~ would give wrong result

Right. if someone will place a tild in windows (in beginning of path) it will be replace with home dir.
Sure. Any wrong path would give wrong result. It replace only the 1st tild. I see this as good solution, any other tild is part of the path.

slavik-lvovsky

This , and other similar, setting property were designed to support only full path that user should provide. A path should be valid because today we do not provide validation for it.
We should probably improve the description of the property but not support all available linux path short hands. Especially when this property is used also in windows.

tomer-epstein · 2021-04-01T09:12:35Z

This , and other similar, setting property were designed to support only full path that user should provide. A path should be valid because today we do not provide validation for it.
We should probably improve the description of the property but not support all available linux path short hands. Especially when this property is used also in windows.

What do u suggest? To drop this fix?
I dont think it is posdible to validate textfield value in the prefrence.

slavik-lvovsky · 2021-04-01T10:08:51Z

This , and other similar, setting property were designed to support only full path that user should provide. A path should be valid because today we do not provide validation for it.
We should probably improve the description of the property but not support all available linux path short hands. Especially when this property is used also in windows.

What do u suggest? To drop this fix?
I dont think it is posdible to validate textfield value in the prefrence.

Need to validate a provided path in exploregens.ts. In case that a path does not exist or invalid we should use the npm public installation path.
The description of the property should explain that the installation path must be valid and absolute.

slavik-lvovsky · 2021-04-04T14:23:38Z

backend/src/exploregens.ts

@@ -18,7 +19,8 @@ export enum GenState {

 export class ExploreGens {
    public static getInstallationLocation(wsConfig: any) {
-        return _.trim(wsConfig.get(ExploreGens.INSTALLATION_LOCATION));
+        let location = _.trim(wsConfig.get(ExploreGens.INSTALLATION_LOCATION));


it should be const

slavik-lvovsky · 2021-04-04T14:25:06Z

backend/tests/exploregens.spec.ts

@@ -60,6 +61,7 @@ import { fail } from "assert";
 describe('exploregens unit test', () => {
    let sandbox: any;


type of sandbox should be SinonSandbox
type of every mock should be SinonMock

it's the same in all our spec files!
https://github.com/SAP/yeoman-ui/blob/DEVXBUGS-8621/backend/tests/yeomanui.spec.ts#L39

Right, it was like this until Alex Gilin found the best option. We should gradually replace all any by right type

slavik-lvovsky · 2021-04-04T14:27:22Z

backend/package.json

@@ -135,7 +135,7 @@
 				},
 				"ApplicationWizard.installationLocation": {
 					"type": "string",
-					"markdownDescription": "Install generators in a specified location. If not set, global location is used."
+					"markdownDescription": "Set the default folder in which generators will be installed. The path to this folder must be valid and absolute. If there is no path defined, the global location will be used."


With this solution we need to mention that the path must exists

approved by Paola.
must be valid --> means it's exist. (valid)

slavik-lvovsky · 2021-04-04T14:27:59Z

backend/tests/exploregens.spec.ts

            const res = exploregens["getGeneratorsLocationParams"]();
            expect(res).to.be.deep.equal(`--prefix ${TESTVALUE}`);
        });
+
+        it("location starts with tild ~", () => {


this test should have fsMock.expects("existsSync").withExactArgs(TESTVALUE).returns(false);

Better not mock it. location is invalid so existSync will return false.

In test environment accidentally this path may exist and the test will fail. It is better to control it with mock

0.01% that accidentally contains a path equal to '~/notExistLocation'

It is always better 100%, especially for tests.

so, in this case it 100% that '~/notExistLocation' will not exist on test system.

slavik-lvovsky · 2021-04-05T05:13:18Z

backend/tests/exploregens.spec.ts

@@ -353,7 +369,7 @@ describe('exploregens unit test', () => {
        });

        it("recommended array is undefined", () => {
-            workspaceConfigMock.expects("get").withExactArgs(exploregens["SEARCH_QUERY"]).returns();
+            workspaceConfigMock.expects("get").withExactArgs(exploregens["SEARCH_QUERY"]).returns("");


it is better to remove .returns("") or .returns() if you do not use the returned value

slavik-lvovsky · 2021-04-05T05:13:30Z

backend/tests/exploregens.spec.ts

@@ -318,22 +323,33 @@ describe('exploregens unit test', () => {
        });

        it("location undefined", () => {
-            workspaceConfigMock.expects("get").withExactArgs(ExploreGens["INSTALLATION_LOCATION"]).returns();
+            workspaceConfigMock.expects("get").withExactArgs(ExploreGens["INSTALLATION_LOCATION"]).returns("");


it is better to remove .returns("") or .returns() if you do not use the returned value

slavik-lvovsky · 2021-04-05T05:13:36Z

backend/tests/exploregens.spec.ts

@@ -214,7 +219,7 @@ describe('exploregens unit test', () => {
            rpcMock.expects("registerMethod").withExactArgs({ func: exploregens["isLegalNoteAccepted"], thisArg: exploregens });
            rpcMock.expects("registerMethod").withExactArgs({ func: exploregens["acceptLegalNote"], thisArg: exploregens });

-            workspaceConfigMock.expects("get").withExactArgs(ExploreGens["INSTALLATION_LOCATION"]).returns();
+            workspaceConfigMock.expects("get").withExactArgs(ExploreGens["INSTALLATION_LOCATION"]).returns("");


it is better to remove .returns("") or .returns() if you do not use the returned value

This reverts commit 589812a.

…535)" This reverts commit 589812a.

…535)" (#539) This reverts commit 589812a.

tomer-epstein requested a review from slavik-lvovsky March 31, 2021 13:20

tomer-epstein force-pushed the DEVXBUGS-8621 branch from 41e662d to 562b0fe Compare March 31, 2021 13:26

fix: install generator to location start with tild ~

801dc63

tomer-epstein force-pushed the DEVXBUGS-8621 branch from 562b0fe to 801dc63 Compare March 31, 2021 13:32

slavik-lvovsky suggested changes Mar 31, 2021

View reviewed changes

slavik-lvovsky reviewed Apr 1, 2021

View reviewed changes

slavik-lvovsky suggested changes Apr 4, 2021

View reviewed changes

tomer-epstein force-pushed the DEVXBUGS-8621 branch 4 times, most recently from 105b46a to 9eca4b6 Compare April 4, 2021 18:42

slavik-lvovsky suggested changes Apr 5, 2021

View reviewed changes

fix: install generator to a valid and absolute location

28c87b2

tomer-epstein force-pushed the DEVXBUGS-8621 branch from 9eca4b6 to 28c87b2 Compare April 5, 2021 07:43

slavik-lvovsky approved these changes Apr 5, 2021

View reviewed changes

tomer-epstein merged commit 589812a into master Apr 5, 2021

tomer-epstein deleted the DEVXBUGS-8621 branch April 5, 2021 08:05

tomer-epstein added a commit that referenced this pull request Apr 5, 2021

Revert "fix: Install generator to location start with tild ~ (#535)"

77d21d8

This reverts commit 589812a.

tomer-epstein mentioned this pull request Apr 5, 2021

Revert "fix: Install generator to location start with tild ~ " #538

Closed

tomer-epstein added a commit that referenced this pull request Apr 5, 2021

Revert "fix: Install generator to location start with tild ~ (#535)"

7f1fdb2

This reverts commit 589812a.

tomer-epstein mentioned this pull request Apr 5, 2021

ci: revert "fix: install generator to a valid and absolute location #539

Merged

tomer-epstein added a commit that referenced this pull request Apr 5, 2021

ci: revert "fix: install generator to a valid and absolute location (#…

216f5af

…535)" This reverts commit 589812a.

tomer-epstein added a commit that referenced this pull request Apr 5, 2021

ci: revert "fix: install generator to a valid and absolute location (#…

38017d9

…535)" (#539) This reverts commit 589812a.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Install generator to location start with tild ~ #535

fix: Install generator to location start with tild ~ #535

tomer-epstein commented Mar 31, 2021

slavik-lvovsky left a comment

tomer-epstein commented Mar 31, 2021 •

edited

Loading

slavik-lvovsky left a comment

tomer-epstein commented Apr 1, 2021 •

edited

Loading

slavik-lvovsky commented Apr 1, 2021

slavik-lvovsky Apr 4, 2021

slavik-lvovsky Apr 4, 2021

tomer-epstein Apr 4, 2021

slavik-lvovsky Apr 4, 2021

slavik-lvovsky Apr 4, 2021

tomer-epstein Apr 4, 2021

slavik-lvovsky Apr 4, 2021

tomer-epstein Apr 4, 2021

slavik-lvovsky Apr 4, 2021

tomer-epstein Apr 4, 2021

slavik-lvovsky Apr 5, 2021

tomer-epstein Apr 5, 2021 •

edited

Loading

slavik-lvovsky Apr 5, 2021

slavik-lvovsky Apr 5, 2021

slavik-lvovsky Apr 5, 2021

		@@ -60,6 +61,7 @@ import { fail } from "assert";
		describe('exploregens unit test', () => {
		let sandbox: any;

fix: Install generator to location start with tild ~ #535

fix: Install generator to location start with tild ~ #535

Conversation

tomer-epstein commented Mar 31, 2021

slavik-lvovsky left a comment

Choose a reason for hiding this comment

tomer-epstein commented Mar 31, 2021 • edited Loading

slavik-lvovsky left a comment

Choose a reason for hiding this comment

tomer-epstein commented Apr 1, 2021 • edited Loading

slavik-lvovsky commented Apr 1, 2021

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

Choose a reason for hiding this comment

tomer-epstein Apr 5, 2021 • edited Loading

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

tomer-epstein commented Mar 31, 2021 •

edited

Loading

tomer-epstein commented Apr 1, 2021 •

edited

Loading

tomer-epstein Apr 5, 2021 •

edited

Loading