Skip to content

robot-simulator: fix test that didn't fail if nothing was thrown#406

Merged
tejasbubane merged 4 commits intoexercism:masterfrom
jscheffner:master
Jun 3, 2018
Merged

robot-simulator: fix test that didn't fail if nothing was thrown#406
tejasbubane merged 4 commits intoexercism:masterfrom
jscheffner:master

Conversation

@jscheffner
Copy link
Copy Markdown
Contributor

@jscheffner jscheffner commented Apr 26, 2018

Solves #407:

The robot-simulator exercise includes a test that should fail if in case of an invalid argument

  • no error message is thrown
  • a wrong message is thrown

In fact, the test only fails in the latter case. So I fixed that. I didn't touch anything else but I really wonder why anybody would throw a String instead of an Error object.

@jscheffner
Copy link
Copy Markdown
Contributor Author

I also updated the example because it didn't implement this behavior.

@tejasbubane
Copy link
Copy Markdown
Member

tejasbubane commented May 12, 2018

Hi @jscheffner Thanks for the PR and sorry for the delay in review. The PR looks good to me 👍

Regarding the custom exception, we had a discussion in #280. We have these string-exceptions scattered across few exercises. We would like to convert them to class-based solutions but just did not find the time to fix all of them. Would you be up to doing it for this one exercise in this PR? You can refer the circular-buffer exercise.

Let me know if otherwise, this PR looks good to go.

@tejasbubane
Copy link
Copy Markdown
Member

@jscheffner Did you get a chance to look at this?

@jscheffner
Copy link
Copy Markdown
Contributor Author

Sorry, I had a lot of other things on my mind. I did change it now, but I'm not sure if in #280 there was consent to use custom error classes or just error objects at all.

@tejasbubane
Copy link
Copy Markdown
Member

This looks good to me 👍

@tejasbubane tejasbubane merged commit c325de5 into exercism:master Jun 3, 2018
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