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

implement setPreference and setPlatformPreference #63

Merged
merged 1 commit into from Apr 14, 2019

Conversation

@crysislinux
Copy link
Contributor

@crysislinux crysislinux commented Jan 26, 2019

Platforms affected

None or All, it will not affect any existing code, just provides new APIs

What does this PR do?

Support set the preference for a specific platform. Currently, only global preference is supported by the cordova-common. We have used the new API to implement a hook which will update platform preference in config.xml

What testing has been done on this change?

Four unit tests are added. The new added tests don't have Test ${number} prefix bc that would lead to a lot of changes in unrelated tests.

@project-bot project-bot bot added this to 🐣 New PR / Untriaged in Apache Cordova: Tooling Pull Requests Jan 26, 2019
Apache Cordova: Tooling Pull Requests automation moved this from 🐣 New PR / Untriaged to ✅ Approved, waiting for Merge Jan 26, 2019
dpogue
dpogue approved these changes Jan 26, 2019
Copy link
Member

@dpogue dpogue left a comment

👍 from me

Loading

@dpogue dpogue merged commit a5cd232 into apache:master Apr 14, 2019
2 checks passed
Loading
Apache Cordova: Tooling Pull Requests automation moved this from ✅ Approved, waiting for Merge to 🏆 Merged, waiting for Release Apr 14, 2019
setPlatformPreference: function (name, platform, value) {
const platformEl = this.doc.find('./platform[@name="' + platform + '"]');
if (!platformEl) {
throw new CordovaError('platform does not exist (received platform: ' + platform + ')');
Copy link
Contributor

@raphinesse raphinesse Apr 14, 2019

Choose a reason for hiding this comment

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

Wouldn't it be nicer if we created the platform element here instead of throwing?

Loading

Copy link
Contributor Author

@crysislinux crysislinux Apr 15, 2019

Choose a reason for hiding this comment

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

I guess it's much simpler to just throw. A new platform won't work automatically by just add a new section in the config.

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Apache Cordova: Tooling Pull Requests
🏆 Merged, waiting for Release
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants