Skip to content
This repository has been archived by the owner on May 10, 2022. It is now read-only.

Various checksum improvements #32

Closed
wants to merge 3 commits into from
Closed

Various checksum improvements #32

wants to merge 3 commits into from

Conversation

intgr
Copy link

@intgr intgr commented Dec 1, 2014

I found three things to quibble about in the checksum implementation, here is the PR.

Long story short, it always bothered me when people write checksum code in such "magic" fashion, comparing the end result with a constant. But in this case it's an actual bug: checksum values '00', '01' and '99' can never occur in valid IBAN numbers, but previously the value 'EE012200221111099080' for example was accepted.

3rd commit refactors the checksum out into a separate function, I found that useful when converting legacy Estonian account numbers to IBAN format. This builds on changes made in the 1st commit.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) when pulling 11cd9ff on intgr:checksum into 47e72fe on benkonrath:master.

This is useful when converting legacy account numbers to IBAN format.
@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) when pulling 226feb7 on intgr:checksum into 47e72fe on benkonrath:master.

@benkonrath
Copy link
Owner

Thanks for the PR. I'm no longer taking pull requests for the IBAN code in this repo as the code has now been contributed to django-localflavor. I will be deprecating this package as soon as the BICField is merged there as well.

I haven't actually looked at this PR but I think it's better to discuss this as part of an improvement to django-localflavor. Can you re-do this PR against the code there? Thanks.

https://github.com/django/django-localflavor

@intgr
Copy link
Author

intgr commented Dec 1, 2014

Sure, thanks. Could you perhaps change the project description and/or README to say explicitly that this project is deprecated?

@intgr intgr closed this Dec 1, 2014
@benkonrath
Copy link
Owner

The package won't be depreciated the BICField is merged in django-localflavor.

@benkonrath
Copy link
Owner

Also the IBAN fields aren't in a released version of django-localflavor yet. As soon as this happens, I'll officially depreciate this module and provide migration instructions.

@benkonrath
Copy link
Owner

I ended up adding a pointer to django-localflavor in the development section of the README. Thanks for the suggestion. I have commit access to django-localflavor so I can review your PR there.

@intgr
Copy link
Author

intgr commented Dec 1, 2014

Thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants