Skip to content

book-store: Make exercise schema-compliant #635

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Mar 7, 2017

Conversation

rpottsoh
Copy link
Member

@rpottsoh rpottsoh commented Mar 6, 2017

Related to #625

@rpottsoh rpottsoh self-assigned this Mar 6, 2017
@rpottsoh rpottsoh requested a review from kotp March 6, 2017 16:47
Copy link
Contributor

@rbasso rbasso left a comment

Choose a reason for hiding this comment

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

Thanks for opening this PR! 👍

I didn't check the test data, because the first commit changes data and formatting at the same time, so it is too hard to see if anything was changed accidentally.

}
}
"cases": [
{
Copy link
Contributor

@rbasso rbasso Mar 6, 2017

Choose a reason for hiding this comment

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

Here there was a description just inside the total key:

returns the total basket price after applying best discount

The way to deal with that in the new schema is to create a test group with that description, so that we don't loose/change any information.

{
  "description": "...",
  "cases": [
    ...
  ]
}

Copy link
Member Author

Choose a reason for hiding this comment

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

@rbasso I was a little confused about how to deal with that description. I added it back in to the second commit at the tail end of comment at the beginning.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, sorry. I didn't see that. 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

@rbasso is that acceptable?
I am not having much luck following your suggestion to add a description. What I add either doesn't satisfy the schema or fails JSONLint.

Copy link
Contributor

@rbasso rbasso Mar 6, 2017

Choose a reason for hiding this comment

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

I guess you chose a hard one to start. 😄

Take a look this link. This is how I would rewrite the test suite, trying to keep the original data organization (before fixing formatting)

Anyway, I guess it is also ok the way it is, because there is just one group, so there will be no confusion.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hey, what can I say? 😄 Thanks for the link. I like what you did there. I am going to update the PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

@rbasso fixed the second commit. Thanks for your help.

@rpottsoh
Copy link
Member Author

rpottsoh commented Mar 6, 2017

@rbasso you have been a json machine today! Thanks! Take a break. 😄

@rbasso
Copy link
Contributor

rbasso commented Mar 6, 2017

@rbasso you have been a json machine today! Thanks! Take a break. 😄

Already stopped. I was in a commit-spree and I didn't see the time passing. 😁

Copy link
Contributor

@rbasso rbasso left a comment

Choose a reason for hiding this comment

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

The indentation is a little irregular at the test data (1 space vs 2-spaces in the rest of the file) but that doesn't hinder readability, so it doesn't need to be changed.

The commit history got a little messed up now. Do you want to change it or should I squash-merge it?

@rpottsoh
Copy link
Member Author

rpottsoh commented Mar 7, 2017 via email

@rbasso rbasso merged commit afc71df into exercism:master Mar 7, 2017
@rpottsoh rpottsoh deleted the book-store branch March 7, 2017 15:02
emcoding pushed a commit that referenced this pull request Nov 19, 2018
generator: use file methods to manipulate filenames :)
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