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

[BREAKING] Make namespace mandatory for application and library projects #430

Merged
merged 9 commits into from
Mar 20, 2020

Conversation

svbender
Copy link
Contributor

BREAKING CHANGE:
UI5 Project must be able to determine the project's namespace,
otherwise an error is thrown.

BREAKING CHANGE:
UI5 Project must be able to determine the project's namespace,
otherwise an error is thrown.
@coveralls
Copy link

coveralls commented Mar 16, 2020

Coverage Status

Coverage increased (+0.1%) to 91.143% when pulling cc69b79 on mandatory-namespace-for-projects into 3f31a5d on master.

svbender and others added 5 commits March 17, 2020 10:35
BREAKING CHANGE:
UI5 Project must be able to determine the project's namespace,
otherwise an error is thrown.
To increase the test coverage again
To increase the test coverage again
getNamespace: from manifest.json with not matching file path
RandomByte
RandomByte previously approved these changes Mar 17, 2020
Copy link
Member

@RandomByte RandomByte left a comment

Choose a reason for hiding this comment

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

Looks good 👍

Please use squash and merge on GitHub to merge this into the master as a single commit

@RandomByte RandomByte dismissed their stale review March 17, 2020 12:38

Actually, since @matz3 is owner of the UI5 Builder, let's wait for his review as well

@@ -284,6 +284,21 @@ test("getDotLibrary: reads correctly", async (t) => {
t.deepEqual(fsPath, expectedPath, ".library fsPath is correct");
});

test("getDotLibrary: reads correctly call again", async (t) => {
Copy link
Member

Choose a reason for hiding this comment

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

What is this testing exactly? The caching mechanism inside getDotLibrary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

Copy link
Member

Choose a reason for hiding this comment

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

But if I remove the caching here:

if (this._pDotLibrary) {
return this._pDotLibrary;
}

The test will still be successful because it doesn't actually test whether something got cached (e.g. no second glob call).


Also, there is already tests for this. It just seem to be incorrect since its missing a second call:

test.serial("getDotLibrary: result is cached", async (t) => {

Copy link
Contributor

Choose a reason for hiding this comment

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

exactly

Copy link
Member

Choose a reason for hiding this comment

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

exactly

...

So we gonna fix the existing test instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

Copy link
Member

@RandomByte RandomByte left a comment

Choose a reason for hiding this comment

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

Please merge using "squash and merge"

@svbender svbender merged commit ee96c00 into master Mar 20, 2020
@svbender svbender deleted the mandatory-namespace-for-projects branch March 20, 2020 12:16
RandomByte added a commit to SAP/ui5-project that referenced this pull request Mar 23, 2020
Tests are failing due to SAP/ui5-builder#430

Adopt tests to provide necessary manifest.json files and expect the
corresponding namespace information.
RandomByte added a commit to SAP/ui5-server that referenced this pull request Mar 23, 2020
Tests are failing due to SAP/ui5-builder#430

 Adopt tests fixture to provide necessary manifest.json file.
RandomByte added a commit to SAP/ui5-project that referenced this pull request Mar 24, 2020
Tests are failing due to SAP/ui5-builder#430

Adopt tests to provide necessary manifest.json files and expect the
corresponding namespace information.
RandomByte added a commit to SAP/ui5-server that referenced this pull request Mar 24, 2020
Tests are failing due to SAP/ui5-builder#430

 Adopt tests fixture to provide necessary manifest.json file.
RandomByte added a commit to SAP/ui5-project that referenced this pull request Mar 24, 2020
Tests are failing due to SAP/ui5-builder#430

Adopt tests to provide necessary manifest.json files and expect the
corresponding namespace information.
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.

4 participants