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

Implement swift support and testing #395

Merged
merged 2 commits into from Aug 26, 2018

Conversation

@knight9999
Copy link
Contributor

@knight9999 knight9999 commented Aug 17, 2018

Platforms affected

ios

What does this PR do?

Add swift support for ios platform and unit testing.
apache/cordova-discuss#107

This PR is needed to be merged first
apache/cordova-common#42 .

What testing has been done on this change?

Run 'cordova build' with this code and cordova-common change.
Run the jasmine tests.

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.
@codecov-io
Copy link

@codecov-io codecov-io commented Aug 17, 2018

Codecov Report

Merging #395 into master will increase coverage by 0.57%.
The diff coverage is 83%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #395      +/-   ##
==========================================
+ Coverage   73.68%   74.26%   +0.57%     
==========================================
  Files          11       12       +1     
  Lines        1463     1562      +99     
==========================================
+ Hits         1078     1160      +82     
- Misses        385      402      +17
Impacted Files Coverage Δ
bin/templates/scripts/cordova/lib/prepare.js 83.71% <100%> (+0.14%) ⬆️
bin/templates/scripts/cordova/Api.js 59.17% <50%> (-1.83%) ⬇️
...in/templates/scripts/cordova/lib/BridgingHeader.js 95.52% <95.52%> (ø)

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 e0d3dad...51d9787. Read the comment docs.

Copy link
Member

@dpogue dpogue left a comment

This all looks good to me, but I'll let someone with more iOS experience give it a more thorough review

this.path = bridgingHeaderPath;
this.bridgingHeaders = null;
if (!fs.existsSync(this.path)) {
throw new CordovaError('BridgingHeader.js is not found.');

This comment has been minimized.

@dpogue

dpogue Aug 17, 2018
Member

This is actually if the BridgingHeader.h file is not found? The error message says .js

This comment has been minimized.

@knight9999

knight9999 Aug 20, 2018
Author Contributor

I am aiming to check the BridgingHeader.h file exists or not.
Sorry I am going to fix the wrong message. Thank you a lot!

bridgingHeaderFile.write();
}
}
return Q.resolve();

This comment has been minimized.

@dpogue

dpogue Aug 17, 2018
Member

Do we need to return Q.resolve() here? I think if the function finishes with no exceptions, it should continue into the next .then block

This comment has been minimized.

@knight9999

knight9999 Aug 20, 2018
Author Contributor

Thank you. There is no reason for Q.resolve(). I will remove it.

bridgingHeaderFile.write();
}
}
return Q.resolve();

This comment has been minimized.

@dpogue

dpogue Aug 17, 2018
Member

Again, I don't think we need to explicitly return Q.resolve() here

This comment has been minimized.

@knight9999

knight9999 Aug 20, 2018
Author Contributor

Thank you. There is no paticular reason for Q.resolve(). I will remove it.

@dpogue
dpogue approved these changes Aug 21, 2018
Copy link
Member

@dpogue dpogue left a comment

👍 from me :)

@knight9999
Copy link
Contributor Author

@knight9999 knight9999 commented Aug 22, 2018

Thanks you!

@knight9999 knight9999 mentioned this pull request Aug 25, 2018
0 of 2 tasks complete
@dpogue dpogue merged commit 2cd9309 into apache:master Aug 26, 2018
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants