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

Updated plugin with new func #17

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

lkounadis
Copy link

Added didFingerprintDatabaseChange to the exposed API of iOS taken from
https://github.com/EddyVerbruggen/cordova-plugin-touch-id

Also changed 2 error codes which were -1 to -11 and -12 as -1 is the same error code you get after 3 unsuccessful tries using touch ID from the iOS

@rhartvig
Copy link

@lkounadis , while the added feature is definitely desired, I don't see why you are introducing a breaking change to the js methods at the same time.

If you think the js method names are so bad that a breaking change is justified, in the process generating a need for updating documentation and any tutorials that may exist, I believe you should add that as a separate pull request. I personally don't.

Thanks for the pull request. I hope it gets approved, but imho preferably without the js breaking changes.

@lkounadis
Copy link
Author

lkounadis commented May 19, 2017 via email

…d method names to be slightly more intuitive
@hendstephen
Copy link

@sjhoeksma @rhartvig

Would you reconsider this merge request, excluding the breaking changes to js function names? I need the ability to detect changes in the fingerprint database, but cannot use the touch-id plugin by Eddy Verbruggen, as both plugins use the window.plugins.touchid namespace.

In my opinion, all commits up to cae2bbc contain the desired feature itself, and would not include the breaking changes to the js function names from commits 3223a5e and d35393c. If you excluded these last two commits you would not break backwards compatibility.

There is also an issue out for this (#33), so I believe others would benefit from this as well. I'm willing to create a new pull request if you decide this is worth including.

@rhartvig
Copy link

@hendstephen , that makes a lot of sense, and I'd feel fine approving a new PR excluding the breaking changes. However, I don't think I have access to do so. In any case, I hope you'll go through with making a new PR and finding someone to approve it.

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