Skip to content
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

diamond: add canonical data #612

Merged
merged 1 commit into from
Mar 1, 2017
Merged

Conversation

stkent
Copy link
Contributor

@stkent stkent commented Feb 26, 2017

Closes #556. See that issue for background information. Please contribute any new comments in this PR. Thanks!

I'm far from a property-based testing expert, so my explanation in the test data comment is likely clumsy. Please feel free to explain how to refine it!

I attempted to follow the proposed canonical data spec from #602. If you spot any deviations from the proposal, please let me know and I can correct them. cc @rbasso for his expertise and validation 😄

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.

I didn't followed the discussion about the tests' contents, so I'm reviewing just the format.

I attempted to follow the proposed canonical data spec from #602. If you spot any deviations from the proposal, please let me know and I can correct them. cc @rbasso for his expertise and validation 😄

I just validated the canonical-data.json file here, so it is formally correct.

@stkent
Copy link
Contributor Author

stkent commented Feb 26, 2017

@rbasso thanks! 👍 @ spec!

@ErikSchierboom
Copy link
Member

@stkent Do you want to modify the canonical-data of this PR (e.g. due to the discussion in #614), or shall we merge it?

@stkent
Copy link
Contributor Author

stkent commented Feb 28, 2017

@ErikSchierboom sorry, I don't quite follow; what was the modification you had in mind? Thanks!

@ErikSchierboom
Copy link
Member

@stkent Oh sorry, I meant the bit about "print". There was some discussion if "print" was a good name, as it might imply that we were printing to the console instead of returning a string.

@stkent
Copy link
Contributor Author

stkent commented Feb 28, 2017

Ah, I'm with you now. Yes, let's change that - easy fix at this point. Incoming!

@stkent stkent force-pushed the diamond-canonical-data branch from a204a3a to 28998dd Compare March 1, 2017 00:00
@stkent
Copy link
Contributor Author

stkent commented Mar 1, 2017

@ErikSchierboom I updated print to rows, but I'm open to other suggestions if that's not clear enough. (I'm thinking that with an updated description and the expected output as context, it should be ok?)

@ErikSchierboom
Copy link
Member

@stkent I think rows is a good choice. 👍

@ErikSchierboom ErikSchierboom merged commit 894a7c6 into master Mar 1, 2017
@stkent stkent deleted the diamond-canonical-data branch March 1, 2017 13:12
emcoding pushed a commit that referenced this pull request Nov 19, 2018
tournament: Regenerate tests based on updated canonical data.
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