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

added support for 19 digit cc #1177

Merged
merged 2 commits into from
Mar 22, 2020
Merged

Conversation

aemarcuss
Copy link
Contributor

added support for 19 digit visa and discover cc

added support for 19 digit visa and  discover
Copy link
Member

@profnandaa profnandaa left a comment

Choose a reason for hiding this comment

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

First, thanks for your PR! 🎉

Could you make these changes:

  • Other that change the current regex for 16-digit CC, can you create another one for 19-digit which can be switched to by checking the string length?
  • Update the tests to cover for the 19-digit case

@profnandaa profnandaa added the 🧹 needs-update For PRs that need to be updated before landing label Oct 25, 2019
@aemarcuss
Copy link
Contributor Author

Hi @profnandaa

do you want some kind of condition to check the length, and if its 19 characters do the new one, if not original one ? the original one is not just for 16 but also supports shorter ones. is your concern about performance or readability ? if its readability maybe breaking it out to match specific cases would be the best ? I have noticed that sometimes there are valid numbers which would not be valid cc number ( based on this https://www.freeformatter.com/credit-card-number-generator-validator.html )

@profnandaa
Copy link
Member

@aemarcuss -- got it. Let it be then. Just add the tests and we should be good to go.

@aemarcuss
Copy link
Contributor Author

@profnandaa added & passed the tests.

@profnandaa profnandaa added 🕑 to-be-reviewed and removed 🧹 needs-update For PRs that need to be updated before landing labels Oct 29, 2019
@aplomBomb
Copy link

@profnandaa You reckon this is good to go?

Copy link
Member

@profnandaa profnandaa left a comment

Choose a reason for hiding this comment

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

Hey, sorry for my delayed check on this one. Thanks for your contrib! 🎉

LGTM.

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.

3 participants