Skip to content

book-store: shortened test descriptions#514

Merged
rpottsoh merged 2 commits intoexercism:masterfrom
rpottsoh:bookstorecanonical
Jan 27, 2017
Merged

book-store: shortened test descriptions#514
rpottsoh merged 2 commits intoexercism:masterfrom
rpottsoh:bookstorecanonical

Conversation

@rpottsoh
Copy link
Copy Markdown
Member

Descriptions have been shortened so as to make the json nicer for automatic test generators, so test case functions do not end up being too long.

Descriptions have been shortened so as to make the json nicer for automatic test generators, so test case functions do not end up being too long.
@kotp
Copy link
Copy Markdown
Member

kotp commented Jan 26, 2017

#470 mentioned shortening the names a few times... Looks like this was known, so happy to see it being addressed.

@rpottsoh rpottsoh self-assigned this Jan 26, 2017
@rpottsoh
Copy link
Copy Markdown
Member Author

@kotp it was discussed in #470 but at the time it wasn't as high of a priority as just coming up with meaningful descriptions. At the time I was not fully aware of how the canonical data was being used, I wasn't aware of automatic test generation at the time.

@Insti
Copy link
Copy Markdown
Contributor

Insti commented Jan 26, 2017

The old array formatting without all those line breaks was nicer.
Having the expected values consistently as floats was better.
It's helpful to order the tests in a progressive fashion. (Empty basket should be the first test)

The shorter descriptions are a step in the right direction, but they still don't communicate why that is a useful thing to be testing.

"Two each of first 3 books and 1 copy each of rest"

Why are we not testing "1 each of the first 3 books and 2 copies of books four and five"?

@Insti
Copy link
Copy Markdown
Contributor

Insti commented Jan 26, 2017

Try and remember to include a link to the problem description when discussing canonical-data files:
https://github.com/exercism/x-common/blob/master/exercises/book-store/description.md

Copy link
Copy Markdown
Contributor

@robkeim robkeim left a comment

Choose a reason for hiding this comment

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

I know it's a matter of style, but personally I find this use of vertical space harder to read than the previous version where all of the books in a basket were on the same line.

From a functional standpoint it looks good, and I like the shortened descriptions.

@ErikSchierboom
Copy link
Copy Markdown
Member

I'm with @robkeim here. The vertical grouping is significantly less readable to me. The descriptions themselves look good to me.

Cleaned up the vertical spacing that was caused by an online JSON editor I was trying.  Introduced `"total"` above `"cases"` along with a `"description"`.
@kotp
Copy link
Copy Markdown
Member

kotp commented Jan 27, 2017

Much better

@ErikSchierboom
Copy link
Copy Markdown
Member

Agreed. This looks great!

@rpottsoh rpottsoh merged commit d0f8912 into exercism:master Jan 27, 2017
@rpottsoh rpottsoh deleted the bookstorecanonical branch January 27, 2017 21:52
emcoding pushed a commit that referenced this pull request Nov 19, 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.

5 participants