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

pascals-triangle: Added tests and more README info. #366

Merged
merged 10 commits into from
Nov 6, 2017

Conversation

hekrause
Copy link
Contributor

While working at this exercise a few days before i thought there should be tests for higher row requests.

I noticed also that the prototype of the rows where where given with a Vec<Vec<u32>> parameter so that at some point the values in the middle should exceed the u32-type-boundaries and a test should panic.

To better understand the principle of the pascals triangle i added another line in the README.

@petertseng
Copy link
Member

Need to ask a favour: For you to submit this pull request to https://github.com/exercism/problem-specifications/blob/master/exercises/pascals-triangle/canonical-data.json and https://github.com/exercism/problem-specifications/blob/master/exercises/pascals-triangle/description.md. If you do not, then when the Rust track uses those files to create its README and tests, these changes you've made will be overwritten and lost. We wouldn't want that to happen.

@petertseng
Copy link
Member

When you make that pull request, I advise that it will be good to prepare a reason for why the following quote from exercism/problem-specifications#362 should be reconsidered:

Fewer Tests

Once a student has gotten 4 rows to work the probably have the algorithm down. No real reason to test larger numbers that I can see.

@petertseng petertseng changed the title Added tests and more README info. pascals-triangle: Added tests and more README info. Oct 18, 2017
@petertseng
Copy link
Member

I needed to add the exercise name to the PR title, otherwise when I look at https://github.com/exercism/rust/pulls I have no idea what this PR is for. Please do this in future PRs

@hekrause
Copy link
Contributor Author

@petertseng I made the requested pull request.(here)

@petertseng
Copy link
Member

Reminder to review this now that the problem-specifications PR is merged.

The version in Cargo.toml should now be 1.2.0

@petertseng petertseng merged commit 7138f7e into exercism:master Nov 6, 2017
@petertseng
Copy link
Member

Very good, thank you.

@petertseng petertseng added the sync/tests Keep a test suite/version in sync with exercism/problem-specifications label Feb 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sync/tests Keep a test suite/version in sync with exercism/problem-specifications
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants