Skip to content

Implementing: isbn-verifier#612

Merged
petertseng merged 1 commit intoexercism:masterfrom
Average-user:isbn
Nov 4, 2017
Merged

Implementing: isbn-verifier#612
petertseng merged 1 commit intoexercism:masterfrom
Average-user:isbn

Conversation

@Average-user
Copy link
Copy Markdown
Member

No description provided.

Comment thread config.json
"core": false,
"unlocked_by": null,
"difficulty": 1,
"topics": [
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

probably make a PR adding strings to all exercises that have it

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It is ok if I make that after this get merged? If that ever happens.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes of course, since doing it after is one of the two possible options:

  1. remove string from this PR, make string PR after this PR is merged
  2. make string PR and it is merged before this PR is merged, in which case keeping string here is OK

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I was thinking on doing the first when I removed Strings.

@petertseng
Copy link
Copy Markdown
Member

I generally think this is good, but I don't like some of the problem-specifications cases so I'm going to get that sorted before merging it here.

@Average-user
Copy link
Copy Markdown
Member Author

Average-user commented Nov 1, 2017

@petertseng What do you mean with sorted? How is that gonna change you disliking it?

@petertseng
Copy link
Copy Markdown
Member

3-598-2X507-0 should be 3-598-2X507-9, otherwise it doesn't check that X is only valid as a check digit

3-598-21507-XA should be 3-598-21507-XX, otherwise it doesn't check too long ISBN

@Average-user
Copy link
Copy Markdown
Member Author

@petertseng But how is that sorting exactly?

@petertseng
Copy link
Copy Markdown
Member

Well, I'm getting it fixed, am I not?

@Average-user
Copy link
Copy Markdown
Member Author

Average-user commented Nov 1, 2017

Ok. Should someone do a PR in the original tests cases?

@petertseng
Copy link
Copy Markdown
Member

Well it looks like I can merge exercism/problem-specifications#993 soon.

@petertseng
Copy link
Copy Markdown
Member

OK exercism/problem-specifications#993 is merged, you can update the test cases now

@Average-user
Copy link
Copy Markdown
Member Author

@petertseng Ok, in a moment

Comment thread exercises/isbn-verifier/package.yaml Outdated
@@ -0,0 +1,20 @@
name: isbn-verifier
version: 1.0.0.1
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

make it 1.1.0.1

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thats because the update ?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Member

@petertseng petertseng left a comment

Choose a reason for hiding this comment

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

ISBN verifier should be good to go now

@petertseng petertseng merged commit 6312ade into exercism:master Nov 4, 2017
@petertseng
Copy link
Copy Markdown
Member

thank you for implementing your seventh exercise!

@Average-user
Copy link
Copy Markdown
Member Author

Thanks to you too. Another one some time?

@petertseng
Copy link
Copy Markdown
Member

Well, whenever you or anyone else sees fit to implement them.

@Average-user Average-user deleted the isbn branch November 5, 2017 21:20
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