search

ZsBT / mrz-java

Machine-Readable Zone parser for Java
70 stars 40 forks source link

fix checkDigit for certain German passports #6

Closed ZsBT closed 7 years ago

ZsBT commented 7 years ago

... where they use FILLER instead of 0 as check digit.

ZsBT commented 7 years ago

As you can see, final has been removed from checkDigit. Did it serve serious purpose? Filler character is replaced with zero. My tests show that now it considers check digit as valid for IRL, GER and the CZE (mentioned in #4 ) passports.

TODO:

mvysny commented 7 years ago

It would be great to have some tests for this functionality. Can you please add more tests for passports you mentioned? Agreed: validcheckdigit definitely must not be static, I missed that, can you please fix that as well? Also I would recommend to use slf4j instead of println; Also validcheckdigit perhaps should be of type mrzrange instead of boolean

ZsBT commented 7 years ago

@mvysny

It would be great to have some tests for this functionality. Can you please add more tests for passports you mentioned?

Sure, I'll put the Czeh and German samples there.

Also I would recommend to use slf4j instead of println;

You're right. I'd better to remove logging however (using in an Android project, the code should be kept as independent as possible).

Agreed: validcheckdigit definitely must not be static, I missed that, can you please fix that as well? Also validcheckdigit perhaps should be of type mrzrange instead of boolean

You might point out that there are more check digits. It is not clean for me how mrzrange can correlate to validated check digits, but here is an idea. Other mrz parsing projects use separate boolean properties for each check digit. What if we added validDocumentNumber, validDateOfBirth, validExpirationDate, validPersonalNumber (booleans) to MrzRecord class?

mvysny commented 7 years ago

Sorry I just found out that I no longer have any time to maintain it. Would you please like to be a maintainer? Just let me know

ZsBT commented 7 years ago

Okay, why not. I'm going to use this heavily in the upcoming year. What are the relations with Innovatrics? Do they own any author rights?

mvysny commented 7 years ago

I don't think so. I mean there already are 15 forks of this project. I'm pretty sure innovatrics doesn't care :-) anyways it's lgpl so its copyleft mechanism allows you quite some rights.

On May 26, 2017 07:14, "ZS" notifications@github.com wrote:

Okay, why not. I'm going to use this heavily in the upcoming year. What are the relations with Innovatrics? Do they own any author rights?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mvysny/mrz-java/pull/6#issuecomment-304186684, or mute the thread https://github.com/notifications/unsubscribe-auth/AAPgiwffNsRmIeYQOgrOvMkcSvdrrjrgks5r9lG0gaJpZM4NmCiR .

ZsBT commented 7 years ago

Personally I don't need any rights, just wanted to know where the original code came from. Now it is clear so I'll add the lgpl licence to the repo instead of my favorite wtfpl. I'm ready to take over the maintenance. Just a practical comment: do you think the best way is to delete your repo? What if you just mention the new one in the readme file? Several forum entries and search hits point to your project.

mvysny commented 7 years ago

Yup, i think the best way is to replace readme with something like "the repository has been moved to XYZ"

On May 27, 2017 09:18, "ZS" notifications@github.com wrote:

Closed #6 https://github.com/mvysny/mrz-java/pull/6.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mvysny/mrz-java/pull/6#event-1099795189, or mute the thread https://github.com/notifications/unsubscribe-auth/AAPgi1mTd0XOf7ZbqyZ1TJ7OUk38NXYcks5r98AggaJpZM4NmCiR .

mvysny commented 7 years ago

Just let me know when you'll​ have the repository ready

On May 27, 2017 09:58, "Martin Vysny" vysny@baka.sk wrote:

Yup, i think the best way is to replace readme with something like "the repository has been moved to XYZ"

On May 27, 2017 09:18, "ZS" notifications@github.com wrote:

Closed #6 https://github.com/mvysny/mrz-java/pull/6.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mvysny/mrz-java/pull/6#event-1099795189, or mute the thread https://github.com/notifications/unsubscribe-auth/AAPgi1mTd0XOf7ZbqyZ1TJ7OUk38NXYcks5r98AggaJpZM4NmCiR .

ZsBT commented 7 years ago

Ok, I'll just fix the above-mentioned issue with that static variable and we could consider my repo as stable.

mvysny commented 7 years ago

Actually it is possible to transfer the repository ownership to you, as per https://help.github.com/articles/about-repository-transfers/ Just delete your fork of mrz-java and let me know once it is deleted - then I'll transfer the repository to you. You'll then receive a mail from github where you need to click the "Accept" link.

ZsBT commented 7 years ago

Good finding, thanks. I've just deleted my repo.