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

Remove GH-547 update for aidl files #561

Closed
wants to merge 1 commit into from

Conversation

brodycj
Copy link
Contributor

@brodycj brodycj commented Nov 15, 2018

As discussed in #547 (comment) this update is needed for https://github.com/j3k0/cordova-plugin-purchase#master (version v7.2.4) to work on Android platform. A quick review would be really appreciated.

@brodycj brodycj mentioned this pull request Nov 15, 2018
7 tasks
@codecov-io
Copy link

codecov-io commented Nov 15, 2018

Codecov Report

Merging #561 into master will decrease coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #561      +/-   ##
==========================================
- Coverage   62.11%   62.07%   -0.04%     
==========================================
  Files          17       17              
  Lines        1985     1983       -2     
  Branches      371      370       -1     
==========================================
- Hits         1233     1231       -2     
  Misses        752      752
Impacted Files Coverage Δ
bin/templates/cordova/lib/pluginHandlers.js 86.98% <ø> (-0.16%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f1396c7...eb868d0. Read the comment docs.

@janpio
Copy link
Member

janpio commented Nov 15, 2018

What does "Remove GH-547 update for aidl files" mean? Does this revert some code from ... what? #547 seems to be an issue...

@brodycj
Copy link
Contributor Author

brodycj commented Nov 15, 2018

GH-547 is an issue that is still open. I already proposed a solution in #550 and it was merged. But when I rebased it on 7.1.x and did some more testing I discovered a case where the changes introduced an issue with a plugin. As I said in the OP the reasoning is explained in #547 (comment).

This PR does the following in a single commit in order to resolve the problem with https://github.com/j3k0/cordova-plugin-purchase#master:

This PR does not revert an entire commit since other changes in the commit (a67bc75) are needed to resolve GH-547.

An alternative solution could be to revert a67bc75, which would remove 10 lines from bin/templates/cordova/lib/pluginHandlers.js along with a few test changes, and then add the 8 lines we do want in a separate commit.

I would be happy either way and open to suggestions. I have been up for over 24 hours and am looking forward to a resolution.

@brodycj
Copy link
Contributor Author

brodycj commented Nov 15, 2018 via email

@janpio
Copy link
Member

janpio commented Nov 15, 2018

Just give this PR here a title and description one can understand without reading your works of the last 48 hours. A PR should be self-contained and have all the information to understand a change 2 years from now. This is currently not the case.

@brodycj
Copy link
Contributor Author

brodycj commented Nov 15, 2018

Closing for now, at least.

@brodycj brodycj closed this Nov 15, 2018
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