rushisangani / BiometricAuthentication

Use Apple FaceID or TouchID authentication in your app using BiometricAuthentication.
MIT License
831 stars 110 forks source link

Carthage, iOS 8 and pt-BR localization. #10

Closed evertoncunha closed 6 years ago

evertoncunha commented 6 years ago

Carthage support; Requirement changed from iOS 9 to iOS 8; New method localizedError() for AuthenticationErrors; Localization for pt-BR.

rushisangani commented 6 years ago

Localization cannot be part of the library. It has to be application specific. Language-specific changes should be part of your application.

evertoncunha commented 6 years ago

@rushisangani I understand your point! The library contains messages in english, which really helps when your app is english only. I just thought we could also help devs with apps in other languages, that's why I have created a new method localizedMessage(), and the current method message() stays english only. Either way the dev will always have the option to customize their own messages, it's just to facilitate.

rushisangani commented 6 years ago

@evertoncunha, I agree with your point. But it's a bad practice to write any language specific code in the library. In this case, you've created Localizable.strings in the library project. So anyways user needs to change the text if they wanted to use some other language.

They've to change in pod project, instead of the application project. I Hope you got my point, Thanks.

evertoncunha commented 6 years ago

@rushisangani I disagree, they would have to write their own implementation of the localized message and just not use the localizedMessage() method, the dev would not need to change the pod code. I don't get it why it's a bad practice to have a localized method in the library, if the point of the library is to help the devs to develop faster. I did Google it as a bad practice and I couldn't find an answer, but that's ok.

So how about if I remove the localization stuff, and just leave the iOS 8 and Carthage support?

rushisangani commented 6 years ago

@evertoncunha I would be very happy to merge your pull request to add Carthage and iOS 8 support. I really appreciate your efforts to make libraries more feature rich.

Thanks.

evertoncunha commented 6 years ago

@rushisangani While I was doing this, I had this issue:

In my app, the error I get when calling .authenticateWithBioMetrics(..) is an AuthenticationError, which is your custom error implementation.

I don't have access to the LAError.code that generated this AuthenticationError, so I can only use the message() and localize the string that it returns to me.

In the future, if we have new error codes, and this library supports it, and then we will have new message() Strings, if someone doesn't pay attention to this in my app, probably some english message will appear to the user.

I do not want to fix the library version in the podfile/cartfile, do you have any suggestions for this?

rushisangani commented 6 years ago

@evertoncunha, I've intentionally kept AuthenticationError instead dealing with complex LAError codes.

As per my testing with all negative scenarios, I've covered all useful codes with specific AuthenticationError value and rest will fall under .other to make the things simpler for developers.

If your authentication failed due to the reason which doesn't fall under specified category consider it as .other and handle that case accordingly in your app.

Apple suggests that if anything does not work then always provide alternate authentication options. I guess in your case you're the best person to decide whether to show an error message to the user or not.

Please feel free to ask if you've any questions.