change: Regenerate Tests#871
Conversation
Update generator to use the standard error indicator. Update solution to pass updated tests.
| skip | ||
| assert_equal -1, Change.generate([5, 10], 3) | ||
| assert_raises(ArgumentError) do | ||
| Change.generate([5, 10], 3) |
There was a problem hiding this comment.
ArgumentError is probably OK here, but what about returning a subclass which is more descriptive?
| skip | ||
| assert_equal -1, Change.generate([5, 10], 94) | ||
| assert_raises(ArgumentError) do | ||
| Change.generate([5, 10], 94) |
There was a problem hiding this comment.
ArgumentError is probably not appropriate here.
Maybe nil or a custom error?
| skip | ||
| assert_equal -1, Change.generate([1, 2, 5], -5) | ||
| assert_raises(ArgumentError) do | ||
| Change.generate([1, 2, 5], -5) |
There was a problem hiding this comment.
ArgumentError is reasonable here. 👍
(I don't think invalid input like this is something we should be testing for, but that's an unrelated issue. (and very much a matter of opinion.))
|
Hello @iHiD! Thank you for your review. Wanted us to deal with #870 before this one, that's why I took so long to answer. I understand and agree with your points, but I have a question: Given how all 3 of these "error cases" are specified on the canonical data as |
|
I assume you meant @Insti there :) Yeah you'll need to do something like that. Either check the test name or error text I guess. |
|
Ah! I was confused :) |
|
@Insti I just pushed some tentative changes. I didn't follow your suggestions to the letter, but wanted to get your opinion regardless. Please let me know what you think. I also appreciate suggestions for better error names 👍 |
pgaspar
force-pushed
the
change-regenerate-tests
branch
from
cb7ff87 to
6c023f5
Compare
| attr_reader :coins, :target | ||
|
|
||
| class NegativeTargetError < ArgumentError; end | ||
| class ImpossibleCombinationError < StandardError; end |
Insti
left a comment
Insti
left a comment
There was a problem hiding this comment.
I think this looks good and does everything it needs to do now.
|
Do you think this is ready to merge now @pgaspar ? |
|
I think so @Insti! Thank you 👍 |
|
Thanks for all your work on these @pgaspar ❤️ |
3 participants
Update
changetests to:1.3.0 258c807Update generator to use the standard error indicator.
Update solution to pass updated tests.