Complete Luhn algorithm #1167
Complete Luhn algorithm #1167
Conversation
|
Looks like build is failing due to an error with netlify, don't worry about that. I'm pretty pissed off with netlify anyway, so will probably move away from them anyway fairly soon, thus not that keen to look into debugging it. @matin originally implemented |
|
Looks good to me. |
Codecov Report
@@ Coverage Diff @@
## master #1167 +/- ##
======================================
Coverage 100% 100%
======================================
Files 20 20
Lines 3490 3509 +19
Branches 679 684 +5
======================================
+ Hits 3490 3509 +19
Continue to review full report at Codecov.
|
|
@cuencandres my recommendation is for you to use the I made a bad choice implementing luhn 10 from scratch vs using a library that already has a solid implementation. |
|
I'd rather not add another requirement for a single use. From a brief look on wikipedia, the luhn algorithm really doesn't look that complicated. Is there really enough complexity to require a separate library? |
|
Nope. It's not complex. That's why I had implemented myself at first. I just missed one part. If you prefer not to have the dependency, I'm ok with the PR as it is now. |
|
+1 for not adding the dependency |
|
Surely for something with as wide-spread use as this algorithm there should be a pretty comprehensive set of tests which pretty much guarantee a correct algorithm? |
|
Update, let's include all of these test cases in this PR. |
|
Works for me. |
|
@cuencandres can you update the tests? Please make sure to use |
|
Sure, I'll work on it. |
|
I added the same tests @samuelcolvin suggested to include +6 suggested by myself. |
changes/1166-cuencandres.rst
Outdated
| @@ -0,0 +1 @@ | |||
| completing Luhn algorithm for ``PaymentCardNumber``. | |||
There was a problem hiding this comment.
| completing Luhn algorithm for ``PaymentCardNumber``. | |
| completing Luhn algorithm for `PaymentCardNumber` |
| VALID_OTHER = '2000000000000000008' | ||
| LUHN_INVALID = '4000000000000000' | ||
| LEN_INVALID = '40000000000000006' | ||
|
|
||
| luhn_tests = [ |
There was a problem hiding this comment.
please move this into the decorator, e.g.
@pytest.mark.parametrize('card_number, valid', [
...
])
|
I'm afraid you've messed up the merge with master. |
|
Maybe easiest to start a new pr? |
|
Sorry, let me create another PR. |
andreshndz
force-pushed
the
payment_card_number
branch
from
349116c to
1a6fdc7
Compare
andreshndz
force-pushed
the
payment_card_number
branch
from
1a6fdc7 to
9dccc37
Compare
|
I've fixed the merge. |
| def test_validate_luhn_check_digit(card_number: str, valid: bool): | ||
| if valid: | ||
| assert PaymentCardNumber.validate_luhn_check_digit(card_number) == card_number | ||
| assert valid |
There was a problem hiding this comment.
| assert valid |
| else: | ||
| with pytest.raises(LuhnValidationError): | ||
| PaymentCardNumber.validate_luhn_check_digit(card_number) | ||
| assert not valid |
There was a problem hiding this comment.
| assert not valid |
|
just one more tiny thing. By the way, somehow you've made your PR so I can't edit it, hence I can't just fix small issues like this. In future it might be worthwhile allowing project collaborators to edit your PRs. |
|
@samuelcolvin , tiny changes are done! |
Change Summary
Luhn algorithm was not complete. An important step of this algorithm was missing.
This PR would solve the problem described in the issue linked below: Luhn Algorithm is not working as expected.
Related issue number
#1166
Checklist
changes/1166-cuencandres.mdfile added describing change(see changes/README.md for details)