You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@cordova.apache.org by GitBox <gi...@apache.org> on 2021/04/18 01:40:34 UTC
[GitHub] [cordova-android] breautek opened a new pull request #1211: enhancement: Control project properties in one place
breautek opened a new pull request #1211:
URL: https://github.com/apache/cordova-android/pull/1211
<!--
Please make sure the checklist boxes are all checked before submitting the PR. The checklist is intended as a quick reference, for complete details please see our Contributor Guidelines:
http://cordova.apache.org/contribute/contribute_guidelines.html
Thanks!
-->
### Platforms affected
Android
### Motivation and Context
<!-- Why is this change required? What problem does it solve? -->
<!-- If it fixes an open issue, please link to the issue here. -->
Currently when we want to bump certain variables such as our gradle version, we have several files we need to keep track of and update. This is an enhancement that allows us to define a single variable such as the gradle version, or our target/min SDK versions in a single place.
### Description
<!-- Describe your changes in detail -->
framework now contains a `defaults.json` file which contains several constants such the gradle version, among other constants which is used throughout both nodejs and gradle to set our defaults values.
Our androidx native unit test also makes use of this file to set its min sdk/target sdk/build tools, etc.
If there is anything else that should be included in the defaults constants I'd be happy to add them.
### Testing
<!-- Please describe in detail how you tested your changes. -->
npm test and manual testing
### Checklist
- [x] I've run the tests to see all new and existing tests pass
- [x] I added automated test coverage as appropriate for this change
- [x] Commit is prefixed with `(platform)` if this change only applies to one platform (e.g. `(android)`)
- [x] If this Pull Request resolves an issue, I linked to the issue in the text above (and used the correct [keyword to close issues using keywords](https://help.github.com/articles/closing-issues-using-keywords/))
- [x] I've updated the documentation if necessary
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org
[GitHub] [cordova-android] breautek commented on pull request #1211: enhancement: Control project properties in one place
Posted by GitBox <gi...@apache.org>.
breautek commented on pull request #1211:
URL: https://github.com/apache/cordova-android/pull/1211#issuecomment-826117018
Closing this in favour of https://github.com/apache/cordova-android/pull/1212
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org
[GitHub] [cordova-android] breautek commented on a change in pull request #1211: enhancement: Control project properties in one place
Posted by GitBox <gi...@apache.org>.
breautek commented on a change in pull request #1211:
URL: https://github.com/apache/cordova-android/pull/1211#discussion_r615404443
##########
File path: framework/cordova.gradle
##########
@@ -155,8 +170,9 @@ def doGetConfigPreference(name, defaultValue) {
ext {
// These helpers are shared, but are not guaranteed to be stable / unchanged.
privateHelpers = {}
+ privateHelpers.getCordovaDefaults = { doGetCordovaDefaults() }
privateHelpers.getProjectTarget = { doGetProjectTarget() }
- privateHelpers.findLatestInstalledBuildTools = { doFindLatestInstalledBuildTools('19.1.0') }
+ privateHelpers.findLatestInstalledBuildTools = { doFindLatestInstalledBuildTools("29.0.2") }
Review comment:
Yah, in fact we already have a variable for it, so good catch
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org
[GitHub] [cordova-android] codecov-commenter edited a comment on pull request #1211: enhancement: Control project properties in one place
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #1211:
URL: https://github.com/apache/cordova-android/pull/1211#issuecomment-821915095
# [Codecov](https://codecov.io/gh/apache/cordova-android/pull/1211?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> :exclamation: No coverage uploaded for pull request base (`master@19bbf1e`). [Click here to learn what that means](https://docs.codecov.io/docs/error-reference?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#section-missing-base-commit).
> The diff coverage is `89.28%`.
[](https://codecov.io/gh/apache/cordova-android/pull/1211?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #1211 +/- ##
=========================================
Coverage ? 71.99%
=========================================
Files ? 22
Lines ? 1696
Branches ? 0
=========================================
Hits ? 1221
Misses ? 475
Partials ? 0
```
| [Impacted Files](https://codecov.io/gh/apache/cordova-android/pull/1211?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...n/templates/cordova/lib/builders/ProjectBuilder.js](https://codecov.io/gh/apache/cordova-android/pull/1211/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YmluL3RlbXBsYXRlcy9jb3Jkb3ZhL2xpYi9idWlsZGVycy9Qcm9qZWN0QnVpbGRlci5qcw==) | `73.85% <0.00%> (ø)` | |
| [...lates/cordova/lib/config/GradlePropertiesParser.js](https://codecov.io/gh/apache/cordova-android/pull/1211/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YmluL3RlbXBsYXRlcy9jb3Jkb3ZhL2xpYi9jb25maWcvR3JhZGxlUHJvcGVydGllc1BhcnNlci5qcw==) | `86.27% <ø> (ø)` | |
| [bin/lib/create.js](https://codecov.io/gh/apache/cordova-android/pull/1211/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YmluL2xpYi9jcmVhdGUuanM=) | `94.90% <100.00%> (ø)` | |
| [bin/templates/cordova/lib/TemplateFile.js](https://codecov.io/gh/apache/cordova-android/pull/1211/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YmluL3RlbXBsYXRlcy9jb3Jkb3ZhL2xpYi9UZW1wbGF0ZUZpbGUuanM=) | `100.00% <100.00%> (ø)` | |
| [bin/templates/cordova/lib/check\_reqs.js](https://codecov.io/gh/apache/cordova-android/pull/1211/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YmluL3RlbXBsYXRlcy9jb3Jkb3ZhL2xpYi9jaGVja19yZXFzLmpz) | `70.71% <100.00%> (ø)` | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/cordova-android/pull/1211?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/cordova-android/pull/1211?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [19bbf1e...ccd1555](https://codecov.io/gh/apache/cordova-android/pull/1211?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org
[GitHub] [cordova-android] codecov-commenter edited a comment on pull request #1211: enhancement: Control project properties in one place
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #1211:
URL: https://github.com/apache/cordova-android/pull/1211#issuecomment-821915095
# [Codecov](https://codecov.io/gh/apache/cordova-android/pull/1211?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> :exclamation: No coverage uploaded for pull request base (`master@b2d9d63`). [Click here to learn what that means](https://docs.codecov.io/docs/error-reference?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#section-missing-base-commit).
> The diff coverage is `88.46%`.
[](https://codecov.io/gh/apache/cordova-android/pull/1211?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #1211 +/- ##
=========================================
Coverage ? 72.14%
=========================================
Files ? 22
Lines ? 1709
Branches ? 0
=========================================
Hits ? 1233
Misses ? 476
Partials ? 0
```
| [Impacted Files](https://codecov.io/gh/apache/cordova-android/pull/1211?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...n/templates/cordova/lib/builders/ProjectBuilder.js](https://codecov.io/gh/apache/cordova-android/pull/1211/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YmluL3RlbXBsYXRlcy9jb3Jkb3ZhL2xpYi9idWlsZGVycy9Qcm9qZWN0QnVpbGRlci5qcw==) | `73.85% <0.00%> (ø)` | |
| [...lates/cordova/lib/config/GradlePropertiesParser.js](https://codecov.io/gh/apache/cordova-android/pull/1211/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YmluL3RlbXBsYXRlcy9jb3Jkb3ZhL2xpYi9jb25maWcvR3JhZGxlUHJvcGVydGllc1BhcnNlci5qcw==) | `86.27% <ø> (ø)` | |
| [bin/lib/create.js](https://codecov.io/gh/apache/cordova-android/pull/1211/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YmluL2xpYi9jcmVhdGUuanM=) | `94.90% <100.00%> (ø)` | |
| [bin/templates/cordova/lib/TemplateFile.js](https://codecov.io/gh/apache/cordova-android/pull/1211/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YmluL3RlbXBsYXRlcy9jb3Jkb3ZhL2xpYi9UZW1wbGF0ZUZpbGUuanM=) | `100.00% <100.00%> (ø)` | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/cordova-android/pull/1211?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/cordova-android/pull/1211?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [b2d9d63...66a0953](https://codecov.io/gh/apache/cordova-android/pull/1211?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org
[GitHub] [cordova-android] breautek commented on a change in pull request #1211: enhancement: Control project properties in one place
Posted by GitBox <gi...@apache.org>.
breautek commented on a change in pull request #1211:
URL: https://github.com/apache/cordova-android/pull/1211#discussion_r615487164
##########
File path: bin/lib/create.js
##########
@@ -69,11 +66,19 @@ function copyJsAndLibrary (projectPath, shared, projectName, isLegacy) {
} else {
fs.ensureDirSync(nestedCordovaLibPath);
fs.copySync(path.join(ROOT, 'framework', 'AndroidManifest.xml'), path.join(nestedCordovaLibPath, 'AndroidManifest.xml'));
- fs.copySync(path.join(ROOT, 'framework', 'project.properties'), path.join(nestedCordovaLibPath, 'project.properties'));
- fs.copySync(path.join(ROOT, 'framework', 'build.gradle'), path.join(nestedCordovaLibPath, 'build.gradle'));
- fs.copySync(path.join(ROOT, 'framework', 'cordova.gradle'), path.join(nestedCordovaLibPath, 'cordova.gradle'));
+ TemplateFile.render(path.join(ROOT, 'framework', 'project.properties'), path.join(nestedCordovaLibPath, 'project.properties'), {
+ DEFAULT_SDK_VERSION: targetAPI || constants.DEFAULT_SDK_VERSION,
+ DEFAULTS_FILE_PATH: './defaults.json'
+ });
+ TemplateFile.render(path.join(ROOT, 'framework', 'build.gradle'), path.join(nestedCordovaLibPath, 'build.gradle'), {
Review comment:
Yah -- I must have forgot to revert. Original (unreleased) version of this PR actually used them, but then I refactored so that the gradle environment can just load in a JSON file itself (which allowed me to also utilize these constants for our test environments)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org
[GitHub] [cordova-android] breautek commented on pull request #1211: enhancement: Control project properties in one place
Posted by GitBox <gi...@apache.org>.
breautek commented on pull request #1211:
URL: https://github.com/apache/cordova-android/pull/1211#issuecomment-826117018
Closing this in favour of https://github.com/apache/cordova-android/pull/1212
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org
[GitHub] [cordova-android] raphinesse commented on a change in pull request #1211: enhancement: Control project properties in one place
Posted by GitBox <gi...@apache.org>.
raphinesse commented on a change in pull request #1211:
URL: https://github.com/apache/cordova-android/pull/1211#discussion_r615382099
##########
File path: bin/templates/cordova/lib/builders/ProjectBuilder.js
##########
@@ -265,10 +265,11 @@ class ProjectBuilder {
}).then(function () {
return self.prepBuildFiles();
}).then(() => {
+ const defaults = this._getCordovaDefaults();
Review comment:
```suggestion
const { DEFAULT_GRADLE_VERSION } = this._getCordovaDefaults();
```
##########
File path: framework/cordova.gradle
##########
@@ -155,8 +170,9 @@ def doGetConfigPreference(name, defaultValue) {
ext {
// These helpers are shared, but are not guaranteed to be stable / unchanged.
privateHelpers = {}
+ privateHelpers.getCordovaDefaults = { doGetCordovaDefaults() }
privateHelpers.getProjectTarget = { doGetProjectTarget() }
- privateHelpers.findLatestInstalledBuildTools = { doFindLatestInstalledBuildTools('19.1.0') }
+ privateHelpers.findLatestInstalledBuildTools = { doFindLatestInstalledBuildTools("29.0.2") }
Review comment:
Is that version not something we can/should move to a config? I'm seriously asking, no idea.
##########
File path: bin/templates/cordova/lib/check_reqs.js
##########
@@ -42,36 +42,19 @@ Object.assign(module.exports, { isWindows, isDarwin });
*/
module.exports.get_target = function () {
const projectPropertiesPaths = [
- path.join(REPO_ROOT, 'framework', 'project.properties'),
+ // path.join(REPO_ROOT, 'framework', 'project.properties'),
path.join(PROJECT_ROOT, 'project.properties')
];
// Get the minimum required target API from the framework.
- let target = projectPropertiesPaths.filter(filePath => fs.existsSync(filePath))
+ const target = projectPropertiesPaths.filter(filePath => fs.existsSync(filePath))
.map(filePath => createEditor(filePath).get('target'))
.pop();
if (!target) {
throw new Error(`We could not locate the target from the "project.properties" at either "${projectPropertiesPaths.join('", "')}".`);
}
- // If the repo config.xml file exists, find the desired targetSdkVersion.
Review comment:
Did you remove a feature here or just change its implementation? Or is this still WIP, seeing the commented-out bits above?
It looks like the removed part was moved to `create.js`?
##########
File path: bin/templates/cordova/lib/builders/ProjectBuilder.js
##########
@@ -287,6 +288,16 @@ class ProjectBuilder {
});
}
+ /**
+ * @private
+ * @returns The defaults file contents
+ */
+ _getCordovaDefaults () {
+ return JSON.parse(fs.readFileSync(path.resolve(this.root, 'defaults.json'), {
+ encoding: 'utf8'
+ }));
Review comment:
`fs-extra` provides helpers for reading & parsing JSON from file:
```suggestion
return fs.readJsonSync(path.resolve(this.root, 'defaults.json'));
```
##########
File path: spec/unit/check_reqs.spec.js
##########
@@ -294,62 +291,63 @@ describe('check_reqs', function () {
}).toThrow();
});
- it('should override target from config.xml preference', () => {
- var realExistsSync = fs.existsSync;
- spyOn(fs, 'existsSync').and.callFake(function (path) {
- if (path.indexOf('config.xml') > -1) {
- return true;
- } else {
- return realExistsSync.call(fs, path);
- }
- });
-
- getPreferenceSpy.and.returnValue(DEFAULT_TARGET_API + 1);
-
- var target = check_reqs.get_target();
-
- expect(getPreferenceSpy).toHaveBeenCalledWith('android-targetSdkVersion', 'android');
- expect(target).toBe('android-' + (DEFAULT_TARGET_API + 1));
- });
-
- it('should fallback to default target if config.xml has invalid preference', () => {
- var realExistsSync = fs.existsSync;
- spyOn(fs, 'existsSync').and.callFake(function (path) {
- if (path.indexOf('config.xml') > -1) {
- return true;
- } else {
- return realExistsSync.call(fs, path);
- }
- });
-
- getPreferenceSpy.and.returnValue(NaN);
-
- var target = check_reqs.get_target();
-
- expect(getPreferenceSpy).toHaveBeenCalledWith('android-targetSdkVersion', 'android');
- expect(target).toBe('android-' + DEFAULT_TARGET_API);
- });
-
- it('should warn if target sdk preference is lower than the minimum required target SDK', () => {
- var realExistsSync = fs.existsSync;
- spyOn(fs, 'existsSync').and.callFake(function (path) {
- if (path.indexOf('config.xml') > -1) {
- return true;
- } else {
- return realExistsSync.call(fs, path);
- }
- });
-
- spyOn(events, 'emit');
-
- getPreferenceSpy.and.returnValue(DEFAULT_TARGET_API - 1);
-
- var target = check_reqs.get_target();
-
- expect(getPreferenceSpy).toHaveBeenCalledWith('android-targetSdkVersion', 'android');
- expect(target).toBe('android-' + DEFAULT_TARGET_API);
- expect(events.emit).toHaveBeenCalledWith('warn', 'android-targetSdkVersion should be greater than or equal to ' + DEFAULT_TARGET_API + '.');
- });
+ // TODO(Breautek) Delete these tests if they are not actually required...
+ // it('should override target from config.xml preference', () => {
+ // var realExistsSync = fs.existsSync;
+ // spyOn(fs, 'existsSync').and.callFake(function (path) {
+ // if (path.indexOf('config.xml') > -1) {
+ // return true;
+ // } else {
+ // return realExistsSync.call(fs, path);
+ // }
+ // });
+
+ // getPreferenceSpy.and.returnValue(DEFAULT_TARGET_API + 1);
+
+ // var target = check_reqs.get_target();
+
+ // expect(getPreferenceSpy).toHaveBeenCalledWith('android-targetSdkVersion', 'android');
+ // expect(target).toBe('android-' + (DEFAULT_TARGET_API + 1));
+ // });
+
+ // it('should fallback to default target if config.xml has invalid preference', () => {
+ // var realExistsSync = fs.existsSync;
+ // spyOn(fs, 'existsSync').and.callFake(function (path) {
+ // if (path.indexOf('config.xml') > -1) {
+ // return true;
+ // } else {
+ // return realExistsSync.call(fs, path);
+ // }
+ // });
+
+ // getPreferenceSpy.and.returnValue(NaN);
+
+ // var target = check_reqs.get_target();
+
+ // expect(getPreferenceSpy).toHaveBeenCalledWith('android-targetSdkVersion', 'android');
+ // expect(target).toBe('android-' + DEFAULT_TARGET_API);
+ // });
+
+ // it('should warn if target sdk preference is lower than the minimum required target SDK', () => {
+ // var realExistsSync = fs.existsSync;
+ // spyOn(fs, 'existsSync').and.callFake(function (path) {
+ // if (path.indexOf('config.xml') > -1) {
+ // return true;
+ // } else {
+ // return realExistsSync.call(fs, path);
+ // }
+ // });
+
+ // spyOn(events, 'emit');
+
+ // getPreferenceSpy.and.returnValue(DEFAULT_TARGET_API - 1);
+
+ // var target = check_reqs.get_target();
+
+ // expect(getPreferenceSpy).toHaveBeenCalledWith('android-targetSdkVersion', 'android');
+ // expect(target).toBe('android-' + DEFAULT_TARGET_API);
+ // expect(events.emit).toHaveBeenCalledWith('warn', 'android-targetSdkVersion should be greater than or equal to ' + DEFAULT_TARGET_API + '.');
+ // });
Review comment:
We should figure out if we need those or not before merging.
##########
File path: bin/templates/cordova/lib/builders/ProjectBuilder.js
##########
@@ -265,10 +265,11 @@ class ProjectBuilder {
}).then(function () {
return self.prepBuildFiles();
}).then(() => {
+ const defaults = this._getCordovaDefaults();
// update/set the distributionUrl in the gradle-wrapper.properties
const gradleWrapperPropertiesPath = path.join(self.root, 'gradle/wrapper/gradle-wrapper.properties');
const gradleWrapperProperties = createEditor(gradleWrapperPropertiesPath);
- const distributionUrl = process.env.CORDOVA_ANDROID_GRADLE_DISTRIBUTION_URL || 'https://services.gradle.org/distributions/gradle-6.8.3-all.zip';
+ const distributionUrl = process.env.CORDOVA_ANDROID_GRADLE_DISTRIBUTION_URL || `https://services.gradle.org/distributions/gradle-${defaults.DEFAULT_GRADLE_VERSION}-all.zip`;
Review comment:
```suggestion
const distributionUrl = process.env.CORDOVA_ANDROID_GRADLE_DISTRIBUTION_URL || `https://services.gradle.org/distributions/gradle-${DEFAULT_GRADLE_VERSION}-all.zip`;
```
##########
File path: framework/gradle/wrapper/gradle-wrapper.properties
##########
@@ -3,4 +3,3 @@ distributionBase=GRADLE_USER_HOME
distributionPath=wrapper/dists
zipStoreBase=GRADLE_USER_HOME
zipStorePath=wrapper/dists
-distributionUrl=https\://services.gradle.org/distributions/gradle-4.6-all.zip
Review comment:
Sure we do not need this anymore? I seriously do not know, just want to bring it to attention.
##########
File path: bin/lib/create.js
##########
@@ -69,11 +66,19 @@ function copyJsAndLibrary (projectPath, shared, projectName, isLegacy) {
} else {
fs.ensureDirSync(nestedCordovaLibPath);
fs.copySync(path.join(ROOT, 'framework', 'AndroidManifest.xml'), path.join(nestedCordovaLibPath, 'AndroidManifest.xml'));
- fs.copySync(path.join(ROOT, 'framework', 'project.properties'), path.join(nestedCordovaLibPath, 'project.properties'));
- fs.copySync(path.join(ROOT, 'framework', 'build.gradle'), path.join(nestedCordovaLibPath, 'build.gradle'));
- fs.copySync(path.join(ROOT, 'framework', 'cordova.gradle'), path.join(nestedCordovaLibPath, 'cordova.gradle'));
+ TemplateFile.render(path.join(ROOT, 'framework', 'project.properties'), path.join(nestedCordovaLibPath, 'project.properties'), {
+ DEFAULT_SDK_VERSION: targetAPI || constants.DEFAULT_SDK_VERSION,
+ DEFAULTS_FILE_PATH: './defaults.json'
+ });
+ TemplateFile.render(path.join(ROOT, 'framework', 'build.gradle'), path.join(nestedCordovaLibPath, 'build.gradle'), {
Review comment:
The only file that seems to actually make use of interpolating template variables is `project.properties`. Did you refactor something and this is leftovers or did I not get something?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org
[GitHub] [cordova-android] breautek closed pull request #1211: enhancement: Control project properties in one place
Posted by GitBox <gi...@apache.org>.
breautek closed pull request #1211:
URL: https://github.com/apache/cordova-android/pull/1211
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org
[GitHub] [cordova-android] breautek closed pull request #1211: enhancement: Control project properties in one place
Posted by GitBox <gi...@apache.org>.
breautek closed pull request #1211:
URL: https://github.com/apache/cordova-android/pull/1211
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org
[GitHub] [cordova-android] codecov-commenter commented on pull request #1211: enhancement: Control project properties in one place
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #1211:
URL: https://github.com/apache/cordova-android/pull/1211#issuecomment-821915095
# [Codecov](https://codecov.io/gh/apache/cordova-android/pull/1211?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> :exclamation: No coverage uploaded for pull request base (`master@19bbf1e`). [Click here to learn what that means](https://docs.codecov.io/docs/error-reference?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#section-missing-base-commit).
> The diff coverage is `89.28%`.
[](https://codecov.io/gh/apache/cordova-android/pull/1211?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #1211 +/- ##
=========================================
Coverage ? 71.99%
=========================================
Files ? 22
Lines ? 1696
Branches ? 0
=========================================
Hits ? 1221
Misses ? 475
Partials ? 0
```
| [Impacted Files](https://codecov.io/gh/apache/cordova-android/pull/1211?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...n/templates/cordova/lib/builders/ProjectBuilder.js](https://codecov.io/gh/apache/cordova-android/pull/1211/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YmluL3RlbXBsYXRlcy9jb3Jkb3ZhL2xpYi9idWlsZGVycy9Qcm9qZWN0QnVpbGRlci5qcw==) | `73.85% <0.00%> (ø)` | |
| [...lates/cordova/lib/config/GradlePropertiesParser.js](https://codecov.io/gh/apache/cordova-android/pull/1211/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YmluL3RlbXBsYXRlcy9jb3Jkb3ZhL2xpYi9jb25maWcvR3JhZGxlUHJvcGVydGllc1BhcnNlci5qcw==) | `86.27% <ø> (ø)` | |
| [bin/lib/create.js](https://codecov.io/gh/apache/cordova-android/pull/1211/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YmluL2xpYi9jcmVhdGUuanM=) | `94.90% <100.00%> (ø)` | |
| [bin/templates/cordova/lib/TemplateFile.js](https://codecov.io/gh/apache/cordova-android/pull/1211/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YmluL3RlbXBsYXRlcy9jb3Jkb3ZhL2xpYi9UZW1wbGF0ZUZpbGUuanM=) | `100.00% <100.00%> (ø)` | |
| [bin/templates/cordova/lib/check\_reqs.js](https://codecov.io/gh/apache/cordova-android/pull/1211/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YmluL3RlbXBsYXRlcy9jb3Jkb3ZhL2xpYi9jaGVja19yZXFzLmpz) | `70.71% <100.00%> (ø)` | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/cordova-android/pull/1211?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/cordova-android/pull/1211?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [19bbf1e...ccd1555](https://codecov.io/gh/apache/cordova-android/pull/1211?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org
[GitHub] [cordova-android] breautek commented on a change in pull request #1211: enhancement: Control project properties in one place
Posted by GitBox <gi...@apache.org>.
breautek commented on a change in pull request #1211:
URL: https://github.com/apache/cordova-android/pull/1211#discussion_r615486204
##########
File path: spec/unit/check_reqs.spec.js
##########
@@ -294,62 +291,63 @@ describe('check_reqs', function () {
}).toThrow();
});
- it('should override target from config.xml preference', () => {
- var realExistsSync = fs.existsSync;
- spyOn(fs, 'existsSync').and.callFake(function (path) {
- if (path.indexOf('config.xml') > -1) {
- return true;
- } else {
- return realExistsSync.call(fs, path);
- }
- });
-
- getPreferenceSpy.and.returnValue(DEFAULT_TARGET_API + 1);
-
- var target = check_reqs.get_target();
-
- expect(getPreferenceSpy).toHaveBeenCalledWith('android-targetSdkVersion', 'android');
- expect(target).toBe('android-' + (DEFAULT_TARGET_API + 1));
- });
-
- it('should fallback to default target if config.xml has invalid preference', () => {
- var realExistsSync = fs.existsSync;
- spyOn(fs, 'existsSync').and.callFake(function (path) {
- if (path.indexOf('config.xml') > -1) {
- return true;
- } else {
- return realExistsSync.call(fs, path);
- }
- });
-
- getPreferenceSpy.and.returnValue(NaN);
-
- var target = check_reqs.get_target();
-
- expect(getPreferenceSpy).toHaveBeenCalledWith('android-targetSdkVersion', 'android');
- expect(target).toBe('android-' + DEFAULT_TARGET_API);
- });
-
- it('should warn if target sdk preference is lower than the minimum required target SDK', () => {
- var realExistsSync = fs.existsSync;
- spyOn(fs, 'existsSync').and.callFake(function (path) {
- if (path.indexOf('config.xml') > -1) {
- return true;
- } else {
- return realExistsSync.call(fs, path);
- }
- });
-
- spyOn(events, 'emit');
-
- getPreferenceSpy.and.returnValue(DEFAULT_TARGET_API - 1);
-
- var target = check_reqs.get_target();
-
- expect(getPreferenceSpy).toHaveBeenCalledWith('android-targetSdkVersion', 'android');
- expect(target).toBe('android-' + DEFAULT_TARGET_API);
- expect(events.emit).toHaveBeenCalledWith('warn', 'android-targetSdkVersion should be greater than or equal to ' + DEFAULT_TARGET_API + '.');
- });
+ // TODO(Breautek) Delete these tests if they are not actually required...
+ // it('should override target from config.xml preference', () => {
+ // var realExistsSync = fs.existsSync;
+ // spyOn(fs, 'existsSync').and.callFake(function (path) {
+ // if (path.indexOf('config.xml') > -1) {
+ // return true;
+ // } else {
+ // return realExistsSync.call(fs, path);
+ // }
+ // });
+
+ // getPreferenceSpy.and.returnValue(DEFAULT_TARGET_API + 1);
+
+ // var target = check_reqs.get_target();
+
+ // expect(getPreferenceSpy).toHaveBeenCalledWith('android-targetSdkVersion', 'android');
+ // expect(target).toBe('android-' + (DEFAULT_TARGET_API + 1));
+ // });
+
+ // it('should fallback to default target if config.xml has invalid preference', () => {
+ // var realExistsSync = fs.existsSync;
+ // spyOn(fs, 'existsSync').and.callFake(function (path) {
+ // if (path.indexOf('config.xml') > -1) {
+ // return true;
+ // } else {
+ // return realExistsSync.call(fs, path);
+ // }
+ // });
+
+ // getPreferenceSpy.and.returnValue(NaN);
+
+ // var target = check_reqs.get_target();
+
+ // expect(getPreferenceSpy).toHaveBeenCalledWith('android-targetSdkVersion', 'android');
+ // expect(target).toBe('android-' + DEFAULT_TARGET_API);
+ // });
+
+ // it('should warn if target sdk preference is lower than the minimum required target SDK', () => {
+ // var realExistsSync = fs.existsSync;
+ // spyOn(fs, 'existsSync').and.callFake(function (path) {
+ // if (path.indexOf('config.xml') > -1) {
+ // return true;
+ // } else {
+ // return realExistsSync.call(fs, path);
+ // }
+ // });
+
+ // spyOn(events, 'emit');
+
+ // getPreferenceSpy.and.returnValue(DEFAULT_TARGET_API - 1);
+
+ // var target = check_reqs.get_target();
+
+ // expect(getPreferenceSpy).toHaveBeenCalledWith('android-targetSdkVersion', 'android');
+ // expect(target).toBe('android-' + DEFAULT_TARGET_API);
+ // expect(events.emit).toHaveBeenCalledWith('warn', 'android-targetSdkVersion should be greater than or equal to ' + DEFAULT_TARGET_API + '.');
+ // });
Review comment:
check_reqs get target was modified, but I think those changes were suppose to be a part of https://github.com/apache/cordova-android/pull/1212
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org
[GitHub] [cordova-android] codecov-commenter edited a comment on pull request #1211: enhancement: Control project properties in one place
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #1211:
URL: https://github.com/apache/cordova-android/pull/1211#issuecomment-821915095
# [Codecov](https://codecov.io/gh/apache/cordova-android/pull/1211?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> :exclamation: No coverage uploaded for pull request base (`master@b2d9d63`). [Click here to learn what that means](https://docs.codecov.io/docs/error-reference?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#section-missing-base-commit).
> The diff coverage is `88.46%`.
[](https://codecov.io/gh/apache/cordova-android/pull/1211?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #1211 +/- ##
=========================================
Coverage ? 72.14%
=========================================
Files ? 22
Lines ? 1709
Branches ? 0
=========================================
Hits ? 1233
Misses ? 476
Partials ? 0
```
| [Impacted Files](https://codecov.io/gh/apache/cordova-android/pull/1211?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...n/templates/cordova/lib/builders/ProjectBuilder.js](https://codecov.io/gh/apache/cordova-android/pull/1211/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YmluL3RlbXBsYXRlcy9jb3Jkb3ZhL2xpYi9idWlsZGVycy9Qcm9qZWN0QnVpbGRlci5qcw==) | `73.85% <0.00%> (ø)` | |
| [...lates/cordova/lib/config/GradlePropertiesParser.js](https://codecov.io/gh/apache/cordova-android/pull/1211/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YmluL3RlbXBsYXRlcy9jb3Jkb3ZhL2xpYi9jb25maWcvR3JhZGxlUHJvcGVydGllc1BhcnNlci5qcw==) | `86.27% <ø> (ø)` | |
| [bin/lib/create.js](https://codecov.io/gh/apache/cordova-android/pull/1211/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YmluL2xpYi9jcmVhdGUuanM=) | `94.90% <100.00%> (ø)` | |
| [bin/templates/cordova/lib/TemplateFile.js](https://codecov.io/gh/apache/cordova-android/pull/1211/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YmluL3RlbXBsYXRlcy9jb3Jkb3ZhL2xpYi9UZW1wbGF0ZUZpbGUuanM=) | `100.00% <100.00%> (ø)` | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/cordova-android/pull/1211?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/cordova-android/pull/1211?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [b2d9d63...66a0953](https://codecov.io/gh/apache/cordova-android/pull/1211?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org
[GitHub] [cordova-android] breautek commented on a change in pull request #1211: enhancement: Control project properties in one place
Posted by GitBox <gi...@apache.org>.
breautek commented on a change in pull request #1211:
URL: https://github.com/apache/cordova-android/pull/1211#discussion_r615404656
##########
File path: framework/gradle/wrapper/gradle-wrapper.properties
##########
@@ -3,4 +3,3 @@ distributionBase=GRADLE_USER_HOME
distributionPath=wrapper/dists
zipStoreBase=GRADLE_USER_HOME
zipStorePath=wrapper/dists
-distributionUrl=https\://services.gradle.org/distributions/gradle-4.6-all.zip
Review comment:
It gets overwritten by the ProjectBuilder if I recall correctly
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org
[GitHub] [cordova-android] breautek commented on pull request #1211: enhancement: Control project properties in one place
Posted by GitBox <gi...@apache.org>.
breautek commented on pull request #1211:
URL: https://github.com/apache/cordova-android/pull/1211#issuecomment-822101601
Thanks @raphinesse for the in-depth review. Obviously I still have some work to do so I set this to a draft for now. I'll try to clean up the PR and address all the concerns you've brought up this weekend coming.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org
[GitHub] [cordova-android] codecov-commenter edited a comment on pull request #1211: enhancement: Control project properties in one place
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #1211:
URL: https://github.com/apache/cordova-android/pull/1211#issuecomment-821915095
# [Codecov](https://codecov.io/gh/apache/cordova-android/pull/1211?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> :exclamation: No coverage uploaded for pull request base (`master@b2d9d63`). [Click here to learn what that means](https://docs.codecov.io/docs/error-reference?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#section-missing-base-commit).
> The diff coverage is `89.28%`.
[](https://codecov.io/gh/apache/cordova-android/pull/1211?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #1211 +/- ##
=========================================
Coverage ? 71.98%
=========================================
Files ? 22
Lines ? 1699
Branches ? 0
=========================================
Hits ? 1223
Misses ? 476
Partials ? 0
```
| [Impacted Files](https://codecov.io/gh/apache/cordova-android/pull/1211?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...n/templates/cordova/lib/builders/ProjectBuilder.js](https://codecov.io/gh/apache/cordova-android/pull/1211/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YmluL3RlbXBsYXRlcy9jb3Jkb3ZhL2xpYi9idWlsZGVycy9Qcm9qZWN0QnVpbGRlci5qcw==) | `73.85% <0.00%> (ø)` | |
| [...lates/cordova/lib/config/GradlePropertiesParser.js](https://codecov.io/gh/apache/cordova-android/pull/1211/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YmluL3RlbXBsYXRlcy9jb3Jkb3ZhL2xpYi9jb25maWcvR3JhZGxlUHJvcGVydGllc1BhcnNlci5qcw==) | `86.27% <ø> (ø)` | |
| [bin/lib/create.js](https://codecov.io/gh/apache/cordova-android/pull/1211/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YmluL2xpYi9jcmVhdGUuanM=) | `94.90% <100.00%> (ø)` | |
| [bin/templates/cordova/lib/TemplateFile.js](https://codecov.io/gh/apache/cordova-android/pull/1211/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YmluL3RlbXBsYXRlcy9jb3Jkb3ZhL2xpYi9UZW1wbGF0ZUZpbGUuanM=) | `100.00% <100.00%> (ø)` | |
| [bin/templates/cordova/lib/check\_reqs.js](https://codecov.io/gh/apache/cordova-android/pull/1211/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YmluL3RlbXBsYXRlcy9jb3Jkb3ZhL2xpYi9jaGVja19yZXFzLmpz) | `70.71% <100.00%> (ø)` | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/cordova-android/pull/1211?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/cordova-android/pull/1211?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [b2d9d63...26897e2](https://codecov.io/gh/apache/cordova-android/pull/1211?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org
[GitHub] [cordova-android] breautek commented on a change in pull request #1211: enhancement: Control project properties in one place
Posted by GitBox <gi...@apache.org>.
breautek commented on a change in pull request #1211:
URL: https://github.com/apache/cordova-android/pull/1211#discussion_r615501719
##########
File path: framework/gradle/wrapper/gradle-wrapper.properties
##########
@@ -3,4 +3,3 @@ distributionBase=GRADLE_USER_HOME
distributionPath=wrapper/dists
zipStoreBase=GRADLE_USER_HOME
zipStorePath=wrapper/dists
-distributionUrl=https\://services.gradle.org/distributions/gradle-4.6-all.zip
Review comment:
ProjectBuilder always overwrites `distributionUrl` via:
https://github.com/apache/cordova-android/blob/66a0953f43bce40697a10fe4cab58419dce96b43/bin/templates/cordova/lib/builders/ProjectBuilder.js#L272-L273
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org
[GitHub] [cordova-android] codecov-commenter edited a comment on pull request #1211: enhancement: Control project properties in one place
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #1211:
URL: https://github.com/apache/cordova-android/pull/1211#issuecomment-821915095
# [Codecov](https://codecov.io/gh/apache/cordova-android/pull/1211?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> :exclamation: No coverage uploaded for pull request base (`master@b2d9d63`). [Click here to learn what that means](https://docs.codecov.io/docs/error-reference?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#section-missing-base-commit).
> The diff coverage is `89.28%`.
> :exclamation: Current head c7e02e0 differs from pull request most recent head 26897e2. Consider uploading reports for the commit 26897e2 to get more accurate results
[](https://codecov.io/gh/apache/cordova-android/pull/1211?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #1211 +/- ##
=========================================
Coverage ? 71.98%
=========================================
Files ? 22
Lines ? 1699
Branches ? 0
=========================================
Hits ? 1223
Misses ? 476
Partials ? 0
```
| [Impacted Files](https://codecov.io/gh/apache/cordova-android/pull/1211?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...n/templates/cordova/lib/builders/ProjectBuilder.js](https://codecov.io/gh/apache/cordova-android/pull/1211/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YmluL3RlbXBsYXRlcy9jb3Jkb3ZhL2xpYi9idWlsZGVycy9Qcm9qZWN0QnVpbGRlci5qcw==) | `73.85% <0.00%> (ø)` | |
| [...lates/cordova/lib/config/GradlePropertiesParser.js](https://codecov.io/gh/apache/cordova-android/pull/1211/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YmluL3RlbXBsYXRlcy9jb3Jkb3ZhL2xpYi9jb25maWcvR3JhZGxlUHJvcGVydGllc1BhcnNlci5qcw==) | `86.27% <ø> (ø)` | |
| [bin/lib/create.js](https://codecov.io/gh/apache/cordova-android/pull/1211/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YmluL2xpYi9jcmVhdGUuanM=) | `94.90% <100.00%> (ø)` | |
| [bin/templates/cordova/lib/TemplateFile.js](https://codecov.io/gh/apache/cordova-android/pull/1211/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YmluL3RlbXBsYXRlcy9jb3Jkb3ZhL2xpYi9UZW1wbGF0ZUZpbGUuanM=) | `100.00% <100.00%> (ø)` | |
| [bin/templates/cordova/lib/check\_reqs.js](https://codecov.io/gh/apache/cordova-android/pull/1211/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YmluL3RlbXBsYXRlcy9jb3Jkb3ZhL2xpYi9jaGVja19yZXFzLmpz) | `70.71% <100.00%> (ø)` | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/cordova-android/pull/1211?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/cordova-android/pull/1211?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [b2d9d63...26897e2](https://codecov.io/gh/apache/cordova-android/pull/1211?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org
[GitHub] [cordova-android] raphinesse commented on a change in pull request #1211: enhancement: Control project properties in one place
Posted by GitBox <gi...@apache.org>.
raphinesse commented on a change in pull request #1211:
URL: https://github.com/apache/cordova-android/pull/1211#discussion_r615592662
##########
File path: bin/lib/create.js
##########
@@ -69,11 +66,19 @@ function copyJsAndLibrary (projectPath, shared, projectName, isLegacy) {
} else {
fs.ensureDirSync(nestedCordovaLibPath);
fs.copySync(path.join(ROOT, 'framework', 'AndroidManifest.xml'), path.join(nestedCordovaLibPath, 'AndroidManifest.xml'));
- fs.copySync(path.join(ROOT, 'framework', 'project.properties'), path.join(nestedCordovaLibPath, 'project.properties'));
- fs.copySync(path.join(ROOT, 'framework', 'build.gradle'), path.join(nestedCordovaLibPath, 'build.gradle'));
- fs.copySync(path.join(ROOT, 'framework', 'cordova.gradle'), path.join(nestedCordovaLibPath, 'cordova.gradle'));
+ TemplateFile.render(path.join(ROOT, 'framework', 'project.properties'), path.join(nestedCordovaLibPath, 'project.properties'), {
+ DEFAULT_SDK_VERSION: targetAPI || constants.DEFAULT_SDK_VERSION,
+ DEFAULTS_FILE_PATH: './defaults.json'
+ });
+ TemplateFile.render(path.join(ROOT, 'framework', 'build.gradle'), path.join(nestedCordovaLibPath, 'build.gradle'), {
Review comment:
OK. I think the JSON file is definitely the better solution :+1:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org