Skip to content

palindrome-products: improve canonical data#1430

Merged
rpottsoh merged 2 commits intoexercism:masterfrom
juhlig:improve_palindome-products
Jan 9, 2019
Merged

palindrome-products: improve canonical data#1430
rpottsoh merged 2 commits intoexercism:masterfrom
juhlig:improve_palindome-products

Conversation

@juhlig
Copy link
Copy Markdown
Contributor

@juhlig juhlig commented Jan 5, 2019

The two tests where the range is invalid (min>max) produce a bad error message, stating "invalid input: min is X and max is Y", which is something of a "go figure". Instead, they should specify the expectation that has been violated, namely that min must be smaller or equal to max.
(Addressed by commit dc69569)

Finding no palindromes in the given range should not be an error, but result in an empty result. The descriptions of the two tests in question even state "empty result for...", for that matter.
(Addressed by commit a4871b4)

@juhlig juhlig requested a review from petertseng January 5, 2019 12:58
@sshine
Copy link
Copy Markdown
Contributor

sshine commented Jan 5, 2019

I'll wait a little with merging in case Peter has more to say.

Comment thread exercises/palindrome-products/canonical-data.json Outdated
@rpottsoh
Copy link
Copy Markdown
Member

rpottsoh commented Jan 5, 2019

Besides my nit picky comment I think this looks good. Thanks for making these changes @juhlig!

@petertseng
Copy link
Copy Markdown
Member

petertseng commented Jan 6, 2019

The changes being proposed: I think the meaningful error message is crucial to have. I am neutral on the empty result change (as long as it's clear that there is no result, I don't really care how it's represented in JSON), so if others want that change, so be it.

There are very good explanations for the changes being proposed in the PR description. I think that these changes should go in the individual commit messages as well - no harm in including them there, right? Even though the one-liner is already understandable, why not include the info in git log?

I also think each commit message should be prefixed with palindrome-products: just as the PR title is

@sshine
Copy link
Copy Markdown
Contributor

sshine commented Jan 7, 2019

null as a generic, empty result seems fine to me.

juhlig added 2 commits January 7, 2019 12:51
The two tests where the range is invalid (min>max) produce a bad error
message, stating "invalid input: min is X and max is Y", which is
something of a "go figure". Instead, they should specify the expectation
that has been violated, namely that min must be smaller or equal to max.
Finding no palindromes in the given range should not be an error, but
result in an empty result.
@juhlig juhlig force-pushed the improve_palindome-products branch from a4871b4 to fbb3047 Compare January 7, 2019 11:55
@juhlig
Copy link
Copy Markdown
Contributor Author

juhlig commented Jan 7, 2019

@petertseng: i prefixed the commit messages with "palindrome-products" and added a body text, much what is given in the PR description.

@rpottsoh rpottsoh merged commit 611082d into exercism:master Jan 9, 2019
@rpottsoh
Copy link
Copy Markdown
Member

rpottsoh commented Jan 9, 2019

Thanks for working on this @juhlig!

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.

4 participants