-
-
Notifications
You must be signed in to change notification settings - Fork 554
luhn: Apply new "input" policy #1054
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
Conversation
exercises/luhn/canonical-data.json
Outdated
"cases": [ | ||
{ | ||
"description": "single digit strings can not be valid", | ||
"property": "valid", | ||
"input": "1", | ||
"input": { | ||
"ccNumber": "1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume ccNumber
stands for creditCardNumber
. If so, why not write it out and use creditCardNumber
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No reason not to other than the description.md also says it could be used for Canadian Social Insurance Numbers. I didn't want the field to be overly long and I couldn't come up with a good, meaningful, name that was generic enough for this first commit.
Yes, ccNumber
stands for creditCardNumber
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aha, I see. Maybe we should then revert to something plain like "input"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"input" inside of "input"? 👎 Technically, I guess, still satisfies #996. How about "aValue"
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really like the "a" prefix. How about just "value"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sold! Thanks @ErikSchierboom.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Per #996