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

CB-13057 : Remove cordova platform save command #586

Closed
wants to merge 3 commits into from

Conversation

audreyso
Copy link
Contributor

@audreyso audreyso commented Aug 1, 2017

Platforms affected

What does this PR do?

Remove cordova platform save command

What testing has been done on this change?

Checklist

  • Reported an issue in the JIRA database
  • Commit message follows the format: "CB-3232: (android) Fix bug with resolving file paths", where CB-xxxx is the JIRA ID & "android" is the platform affected.
  • Added automated test coverage as appropriate for this change.

@audreyso
Copy link
Contributor Author

audreyso commented Aug 1, 2017

Can I delete:

  1. Commented out tests or parts of tests that contain platforms.json or platform_metadata?
  2. Can I delete save.js and save.spec.js?

Copy link
Contributor

@stevengill stevengill left a comment

Choose a reason for hiding this comment

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

This looks good overall. I like that we won't need platforms.JSON files anymore

module.exports.getPlatformVersions = getVersions;
module.exports.save = save;
module.exports.remove = remove;
// /**
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you mean to comment out this whole file? Might as well just delete it if that is the case

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like these functions were being used by the save command, hence the comment out

var cordova_util = require('../util');
var ConfigParser = require('cordova-common').ConfigParser;
var platformMetadata = require('../platform_metadata');
// var semver = require('semver');
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets delete this whole file as it seems like it is all commented out

@@ -79,7 +79,7 @@ function remove (hooksRunner, projectRoot, targets, opts) {
// Remove targets from platforms.json.
targets.forEach(function (target) {
events.emit('verbose', 'Removing platform ' + target + ' from platforms.json file...');
platformMetadata.remove(projectRoot, target);
// platformMetadata.remove(projectRoot, target);
Copy link
Contributor

Choose a reason for hiding this comment

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

delete

@@ -25,7 +25,7 @@ var events = require('cordova-common').events;
var npmUninstall = require('cordova-fetch').uninstall;
var cordova_util = require('../util');
var config = require('../config');
var platformMetadata = require('../platform_metadata');
// var platformMetadata = require('../platform_metadata');
Copy link
Contributor

Choose a reason for hiding this comment

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

delete

case 'save':
events.emit('warn', 'This command has been deprecated and will be removed in the next major release of cordova.');
return module.exports.save(hooksRunner, projectRoot, opts);
// case 'save':
Copy link
Contributor

Choose a reason for hiding this comment

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

delete

@@ -142,8 +142,8 @@ describe('tests platform/spec restore with --save', function () {
var cwd = process.cwd();
var pkgJsonPath = path.join(cwd, 'package.json');
var pkgJson;
var platformsFolderPath = path.join(cwd, 'platforms/platforms.json');
var platformsJson;
//var platformsFolderPath = path.join(cwd, 'platforms/platforms.json');
Copy link
Contributor

Choose a reason for hiding this comment

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

delete all the stuff that you commented in this file

@@ -621,15 +621,15 @@ describe('During add, if pkg.json has a platform/plugin spec, use that one.', fu
var iosVersion;
var cwd = process.cwd();
var iosDirectory = path.join(cwd, 'platforms/ios/cordova/version');
var platformsFolderPath = path.join(cwd, 'platforms/platforms.json');
//var platformsFolderPath = path.join(cwd, 'platforms/platforms.json');
Copy link
Contributor

Choose a reason for hiding this comment

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

delete

@@ -24,7 +24,7 @@ var events = require('cordova-common').events;
var rewire = require('rewire');
var platform_addHelper = rewire('../../../src/cordova/platform/addHelper');
var platform_module = require('../../../src/cordova/platform');
var platform_metadata = require('../../../src/cordova/platform_metadata');
// var platform_metadata = require('../../../src/cordova/platform_metadata');
Copy link
Contributor

Choose a reason for hiding this comment

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

delete this + other commented out code

fail('did not expect fail handler to be invoked');
}).done(done);
});
// it('should direct `save` commands to the `save` method/module', function (done) {
Copy link
Contributor

Choose a reason for hiding this comment

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

delete

@@ -21,7 +21,7 @@ var Q = require('q');
var events = require('cordova-common').events;
var rewire = require('rewire');
var platform_remove = rewire('../../../src/cordova/platform/remove');
var platform_metadata = require('../../../src/cordova/platform_metadata');
// var platform_metadata = require('../../../src/cordova/platform_metadata');
Copy link
Contributor

Choose a reason for hiding this comment

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

delete this + other commented out code comments

@audreyso audreyso force-pushed the CB-13057-2 branch 4 times, most recently from ad3dcfa to df6f0a9 Compare December 13, 2017 00:12
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.

2 participants