search

sjhoeksma / cordova-plugin-keychain-touch-id

Touch ID plugin with saving password in keychain for IOS and android
87 stars 160 forks source link

Updated plugin with new func #17

Open lkounadis opened 7 years ago

lkounadis commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

thanks for your comment, I actually disagreed with one particular method name which is delete however you are right. these are braking changes. I will revert them asap.

On 19 May 2017 at 10:55, norpoint notifications@github.com wrote:

@lkounadis https://github.com/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.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/sjhoeksma/cordova-plugin-keychain-touch-id/pull/17#issuecomment-302635566, or mute the thread https://github.com/notifications/unsubscribe-auth/AFko8E5g36bqST4F_nTlmadE7ZBxudkGks5r7Ur2gaJpZM4NYKI5 .

hendstephen commented 5 years ago

@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 cae2bbc1d10c48bb62c65491b5c3e48d25d065b1 contain the desired feature itself, and would not include the breaking changes to the js function names from commits 3223a5e50053daa65dbde6c57784d479531c1967 and d35393c801465c4fe038887ba736b328069c3311. 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 commented 5 years ago

@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.