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

add Dankort support #106

Closed
wants to merge 3 commits into from
Closed

add Dankort support #106

wants to merge 3 commits into from

Conversation

cassiocardoso
Copy link

@cassiocardoso cassiocardoso commented May 2, 2020

Hi!

I was working on integrating this lib into another one that I'm maintaining and noticed that you had the Dankort issue flag tagged as help wanted, so here it is. 😄

Since the changes are mostly in the card configuration and docs, I think this won't have a negative impact on the TS migration you've been working on.

Let me know if there's anything else needed.

Proposal

Add Dankort support. (closes #100)

Details

  • Add Dankort card type configuration
  • Update tests for detecting the new card type
  • Update README with the relevant information for the Dankort type.

Comment on lines +137 to +138
[5000, 5018],
[502000, 506698],
Copy link
Author

Choose a reason for hiding this comment

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

I split the patterns here to make 5019 Dankort-exclusive. This didn't affect the other unit tests for Maestro.

@crookedneighbor
Copy link
Contributor

We can't accept this without official documentation about the dankort ranges. Please provide that.

@cassiocardoso
Copy link
Author

Hey @crookedneighbor I thought this references would be enough from your comments on the thread

@crookedneighbor
Copy link
Contributor

LOL, you're right. Totally forgot the reference was in the original issue.

@cassiocardoso
Copy link
Author

haha no worries :)

Copy link
Contributor

@gesa gesa left a comment

Choose a reason for hiding this comment

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

Code looks good, but given that we're about to merge a PR converting this library to typescript, i'd loooove if these changes could be updated after we flip that switch

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
cassiocardoso and others added 2 commits May 5, 2020 09:21
@cassiocardoso
Copy link
Author

Hi @gesa, thanks for your review! Sounds good to me, let me know once the TS migration is merged and I can update this one :)

@crookedneighbor
Copy link
Contributor

The typescript migration is complete.

@crookedneighbor
Copy link
Contributor

Going to close this for now, but once you've merged in master, feel free to open a new PR.

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.

Dankort is not recognized
3 participants