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

Rewrite install dir resolution for legacy plugins #589

Merged
merged 8 commits into from
Nov 28, 2018

Conversation

Jule-
Copy link
Contributor

@Jule- Jule- commented Nov 27, 2018

Let's start fresh!

What does this PR do?

This is the rebase of my old PR #576 coming from #575 on the master branch as encouraged.
Basically I have separated bloc that was handling new app directory structure from the old one. Plus I have rewrite tests with RegExp allowing buggy forms with or without / at the end.
Plus all (hope so) code reviews from @brodybits.

What testing has been done on this change?

  • new: app, app/, app/whatever
  • old: src, src/, src/whatever/source, libs, libs/, libs/whatever/bin
  • buggy: src/myappsomething/source, appsomething/source
    Like my original issue with: cordova-plugin-local-notifications. Try it:
$ cordova plugin add cordova-plugin-local-notification

Contextual issues and PRs raised

#577, #578, #580

@codecov-io
Copy link

codecov-io commented Nov 27, 2018

Codecov Report

Merging #589 into master will increase coverage by 0.13%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #589      +/-   ##
==========================================
+ Coverage   62.11%   62.24%   +0.13%     
==========================================
  Files          17       17              
  Lines        1985     1992       +7     
  Branches      371      371              
==========================================
+ Hits         1233     1240       +7     
  Misses        752      752
Impacted Files Coverage Δ
bin/templates/cordova/lib/pluginHandlers.js 87.64% <100%> (+0.5%) ⬆️

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 ef24341...4041763. Read the comment docs.

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.

Can you make a couple more vars for:

  • '\\' + PATH_SEPARATOR
  • (\\' + PATH_SEPARATOR + '|$)

Everything else looks good now, I will merge if you fix it right. Thanks again for your efforts.

@Jule-
Copy link
Contributor Author

Jule- commented Nov 28, 2018

Is that what you were thinking about?

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.

Merging now. Thanks @Jule- for contributing the improvements.

@Jule-
Copy link
Contributor Author

Jule- commented Nov 28, 2018

You're welcome! 🙂👍

@brodycj brodycj merged commit 8a4ae31 into apache:master Nov 28, 2018
@brodycj
Copy link
Contributor

brodycj commented Nov 28, 2018

This change is merged onto the master branch by a "squash merge", may be included on 7.1.x branch if we need it someday. I edited a few of the listed changes for extra clarity while merging.

@Jule- I noticed that the changes were proposed from your master branch. In the future you may want to propose changes from a new branch name to keep your own master branch clean; this is also better in case we merge with a merge commit.

@Jule-
Copy link
Contributor Author

Jule- commented Nov 28, 2018

@Jule- I noticed that the changes were proposed from your master branch. In the future you may want to propose changes from a new branch name to keep your own master branch clean; this is also better in case we merge with a merge commit.

Copy that!

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