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

Feat: enchance isCreditCard #2008

Merged
merged 2 commits into from
Oct 17, 2022
Merged

Conversation

brianwhaley
Copy link
Contributor

Add regex validation for AmEx card

Copy link
Member

@rubiin rubiin left a comment

Choose a reason for hiding this comment

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

hey there, instead of making a case specific validator, I would suggest something like isDebitcard or isCreditCard followed by provider as a param

@brianwhaley
Copy link
Contributor Author

brianwhaley commented Jul 27, 2022

hey @rubiin i have refactored the isCreditCard file, will be submitting just submitted a new PR shortly. Thanks for you suggestions!

@codecov
Copy link

codecov bot commented Jul 27, 2022

Codecov Report

Merging #2008 (0eaded6) into master (1bb14e8) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master     #2008   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          104       104           
  Lines         2203      2210    +7     
  Branches       477       481    +4     
=========================================
+ Hits          2203      2210    +7     
Impacted Files Coverage Δ
src/lib/isCreditCard.js 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Member

@rubiin rubiin left a comment

Choose a reason for hiding this comment

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

Looks good, lets wait for others

@rubiin rubiin changed the title Feat/isAmexCard Feat/isCreditCard Jul 28, 2022
@brianwhaley
Copy link
Contributor Author

Thank you @rubiin. @profnandaa when you have an opportunity please do look at the PR. Much appreciated!

@brianwhaley brianwhaley deleted the feat/isAmexCard branch July 28, 2022 13:18
@brianwhaley brianwhaley restored the feat/isAmexCard branch July 28, 2022 15:50
@brianwhaley brianwhaley reopened this Jul 28, 2022
@brianwhaley
Copy link
Contributor Author

apologies, thought this was approved and started cleaning up too soon. PR is reopened and waiting for approval from @profnandaa

@rubiin rubiin self-requested a review July 28, 2022 17:39
@rubiin rubiin added ready-to-land For PRs that are reviewed and ready to be landed and removed needs-more-review 🍿 discussion labels Jul 28, 2022
Copy link
Member

@rubiin rubiin left a comment

Choose a reason for hiding this comment

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

I found out that we already have isCreditCard. You can enhance it by adding your changes. We might have to consider releasing this on next major release as it would introduce a breaking change

@rubiin rubiin changed the title Feat/isCreditCard Feat: enchance isCreditCard Jul 28, 2022
@brianwhaley
Copy link
Contributor Author

@rubiin - i did modify the existing isCreditCard file, and ensured it was backwards compatible. can you evaluate the diff? you will see you can still pass in a credit card number without the second param, and still get the same result. Thanks

rubiin
rubiin previously approved these changes Jul 28, 2022
Copy link
Member

@rubiin rubiin left a comment

Choose a reason for hiding this comment

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

LGTM

@brianwhaley
Copy link
Contributor Author

Hi @rubiin and @profnandaa - is there an additional person that can approve this PR? I would like to get it merged to main. Thank you!

@rubiin
Copy link
Member

rubiin commented Jul 29, 2022

Lets wait for @profnandaa

@brianwhaley
Copy link
Contributor Author

@profnandaa and @rubiin , is there an additional person that can approve this PR? I would like to get it merged to main. Thank you!

@brianwhaley
Copy link
Contributor Author

@WikiRik would you be able to review this PR ? Thank you!

@WikiRik
Copy link
Member

WikiRik commented Aug 10, 2022

@brianwhaley I can, but my review won't get this merged faster unfortunately. We'll have to wait for @profnandaa

Either way you can still use this specific commit in other projects, see this NPM docs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🎉 first-pr ✅ LGTM ready-to-land For PRs that are reviewed and ready to be landed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants