Skip to content

Implement isbn-verifier exercise #485

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

Merged
merged 2 commits into from
Nov 10, 2017

Conversation

jenlouie
Copy link
Contributor

@jenlouie jenlouie commented Nov 9, 2017

Closes #484

  • Added example code
  • Added test generator plus tests
  • Added entry in config.json

Please let me know if there's anything I'm missing! Thanks!

* Added example code
* Added test generator plus tests
* Added entry in config.json
Copy link
Member

@ErikSchierboom ErikSchierboom left a comment

Choose a reason for hiding this comment

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

Just one small nit, other than that this is brilliant!

{
protected override void UpdateCanonicalData(CanonicalData canonicalData)
{
canonicalData.Exercise = "Isbn";
Copy link
Member

Choose a reason for hiding this comment

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

In general, I would like to conform to the canonical data. Could you remove this line? This will cause the Isbn class to be renamed to IsbnVerifier, which still makes sense to me (and matches the file name).

canonicalData.Exercise = "Isbn";
foreach (var canonicalDataCase in canonicalData.Cases)
{
canonicalDataCase.Property = "IsValid";
Copy link
Member

Choose a reason for hiding this comment

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

Much better than the existing property! In fact, I've gone and submitted a PR to change the canonical data.

@jenlouie
Copy link
Contributor Author

@ErikSchierboom, thanks for the review!

@ErikSchierboom ErikSchierboom merged commit 287af69 into exercism:master Nov 10, 2017
@ErikSchierboom
Copy link
Member

Merged! Thanks a lot! 🎉

@jenlouie jenlouie deleted the exercise-isbn-verifier branch November 10, 2017 18:16
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.

2 participants