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

[FEATURE] Add ui5Framework translator and resolvers #265

Merged
merged 49 commits into from
Mar 10, 2020

Conversation

matz3
Copy link
Member

@matz3 matz3 commented Feb 17, 2020

Thank you for your contribution! 🙌

To get it merged faster, kindly review the checklist below:

Pull Request Checklist

@coveralls
Copy link

coveralls commented Feb 18, 2020

Coverage Status

Coverage increased (+4.4%) to 88.246% when pulling fbf51b6 on ui5Framework-translator into 34b543b on master.

lib/ui5Framework/AbstractResolver.js Outdated Show resolved Hide resolved
lib/ui5Framework/AbstractResolver.js Outdated Show resolved Hide resolved
lib/ui5Framework/AbstractResolver.js Outdated Show resolved Hide resolved
lib/ui5Framework/AbstractResolver.js Outdated Show resolved Hide resolved
@RandomByte RandomByte marked this pull request as ready for review March 6, 2020 13:25
lib/normalizer.js Outdated Show resolved Hide resolved
lib/ui5Framework/AbstractResolver.js Outdated Show resolved Hide resolved
@matz3 matz3 force-pushed the ui5Framework-translator branch 2 times, most recently from b3e8875 to c151c9d Compare March 9, 2020 12:19
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.

Nice tests 🥇

lib/ui5Framework/AbstractResolver.js Outdated Show resolved Hide resolved
lib/ui5Framework/AbstractResolver.js Outdated Show resolved Hide resolved
lib/ui5Framework/AbstractResolver.js Outdated Show resolved Hide resolved
lib/ui5Framework/Sapui5Resolver.js Outdated Show resolved Hide resolved
lib/ui5Framework/npm/Installer.js Outdated Show resolved Hide resolved
test/lib/translators/ui5Framework.integration.js Outdated Show resolved Hide resolved
lib/ui5Framework/npm/Installer.js Show resolved Hide resolved
test/lib/translators/ui5Framework.js Show resolved Hide resolved
test/lib/ui5framework/Sapui5Resolver.js Show resolved Hide resolved
test/lib/ui5framework/npm/Installer.js Outdated Show resolved Hide resolved
matz3 added 22 commits March 10, 2020 17:22
toJSON is actually required
Also log some more relevant npm config values
An error was detected while testing with a path that is not explicitly
absolute or relative, which results into requiring from node_modules.
e.g. require("path/to/metadata.json") would try to find
"to/metadata.json" within the "path" package.

This might be solve by using path.resolve on the path passed to require.
But this still leaves a problem open, which is a secuirity risk of
requiring a file that is dynamically downloaded. If it doesn't contain
JSON but JavaScript it would be executed directly.

Therefore, using fs.readFile + JSON.parse is a much better approach.
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.

veri nyce 🌮

Please squash and merge

@matz3 matz3 merged commit 5183e5c into master Mar 10, 2020
@matz3 matz3 deleted the ui5Framework-translator branch March 10, 2020 16:56
@matz3 matz3 changed the title [FEATURE] Add ui5Framework translator [FEATURE] Add ui5Framework translator and resolvers Mar 10, 2020
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