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

Cherry picks #549

Closed
wants to merge 2 commits into from
Closed

Cherry picks #549

wants to merge 2 commits into from

Conversation

jcesarmobile
Copy link
Member

Platforms affected

android

What does this PR do?

cherry-picks #539 and #540 fixes on 7.1.x branch

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.

Copy link
Contributor

@brodycj brodycj left a comment

Choose a reason for hiding this comment

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

In general I am not so happy that the unit test updates from PR #542 were not included in this cherry-pick proposal. The unit tests are there to help us ensure that it doesn't get broken in the future.

if (options && options.android_studio === true) {
dest = studioPathRemap(obj);
}
var dest = getInstallDestination(obj);
Copy link
Contributor

Choose a reason for hiding this comment

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

I would downvote this change in a patch release, due to the potential to cause an upgrade issue. I would make a much simpler change here:

@@ -34,7 +34,7 @@ var handlers = {
             // a later plugins release.  This is for legacy plugins to work with Cordova.
 
             if (options && options.android_studio === true) {
-                dest = studioPathRemap(obj);
+                dest = getInstallDestination(obj);
             }
 
             if (options && options.force) {

if (options && options.android_studio === true) {
dest = studioPathRemap(obj);
}
var dest = getInstallDestination(obj);
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as I made on line 31. Here is a much simpler change:

@@ -47,7 +47,7 @@ var handlers = {
             var dest = path.join(obj.targetDir, path.basename(obj.src));
 
             if (options && options.android_studio === true) {
-                dest = studioPathRemap(obj);
+                dest = getInstallDestination(obj);
             }
 
             // TODO: Add Koltin extension to uninstall, since they are handled like Java files

Copy link
Member Author

Choose a reason for hiding this comment

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

if (options && options.android_studio === true) is going to be true always, so there is no point on keeping it

@brodycj
Copy link
Contributor

brodycj commented Nov 13, 2018

Thanks @jcesarmobile for your efforts. I am actually in the process of raising another PR to resolve GH-547, and I have already unit-tested a rebase of my fixes (GH-540 and GH-547) on 7.1.x. I would be happy to raise the rebase as a WIP PR for discussion and review.

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