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

Calculate Italian bban checksum code and fix Italian iban generation #26

Merged
merged 3 commits into from
Apr 8, 2020

Conversation

alitaker
Copy link

No description provided.

@mdomke
Copy link
Owner

mdomke commented Mar 31, 2020

Hi @alitaker,
I'm not quite sure why the result of the CI pipeline did not get propagated back to this pull-request (I'm gonna look into that), but there are several things that the automatic testing/format-checking did discover (https://travis-ci.org/github/mdomke/schwifty/builds/666876707). Can you please have a look at that?
Thanks for your contribution!

Copy link
Owner

@mdomke mdomke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check the development-section of the README. We use black as code-formatter to avoid any discussions about style, because everybody has a different opinion about this…

schwifty/iban.py Outdated Show resolved Hide resolved
schwifty/iban.py Outdated Show resolved Hide resolved
schwifty/iban.py Outdated Show resolved Hide resolved
schwifty/iban.py Outdated Show resolved Hide resolved
@alitaker alitaker requested a review from mdomke April 8, 2020 12:20
@alitaker
Copy link
Author

alitaker commented Apr 8, 2020

Do you think it's ok now?

@mdomke mdomke merged commit e752ddd into mdomke:master Apr 8, 2020
@mdomke
Copy link
Owner

mdomke commented Apr 8, 2020

@alitaker Even though this is already merged: Are you sure this implementation is actually correct? It looks a little different to this one https://github.com/globalcitizen/php-iban/blob/master/php-iban.php#L1241 Do you maybe have a resource that explains the used algorithm?

@alitaker
Copy link
Author

alitaker commented Apr 9, 2020

@mdomke I got the information from here:
http://alexandrerodichevski.chiappani.it/doc.php?n=218&lang=it#algo (sorry, it's in Italian).
They are very similar, the only larger difference is that the even digits just use the index for my implementation, while the link you're referring to uses an array of even indices, which are... the indices themselves! (from 1 to 28).
The same for the letters array, which are calculated using the ascii value in the implementation I wrote.
I find the version you linked more elengant, but both will work.

You may test them using the following website for free:
https://it.ibancalculator.com/iban_validieren.html

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.

2 participants