Changes test assertions to compare wrapped values

[nfiles]

Resolves #338

Test assertions should compare Option or Result values directly instead of unwrapping them.

[petertseng]

accidental removal of space?

[petertseng]

the addition of a space is good, but isn't related to removing unwrap, so it should go in a separate commit

[nfiles]

I have committed a revised changeset that reverts the spacing issues. :)

[petertseng]

Fantastic, thanks for taking the time to do this.

Please forgive a few silly questions:

After this is merged, are there any remaining instances of unwrap that should be changed? I do know there are other instances of unwrap, but not necessarily whether they should be changed.

If there are not, it would be appropriate to close #338 and you should use one of https://help.github.com/articles/closing-issues-using-keywords/ in your commit message and/or pull request description.

If there are, I'll take your word for it. It would be great, but not required, if you would briefly describe what's left to do (I assume you looked through a few of them while making this PR).

About all those as_slice that turned into vec!, I think Vec was in fact the only type the optional type could have taken on, correct? So we are not unnecessarily restricting anyone's solution to using Vecs when they could have used some other type instead, right? A quick search on my part tells me this is true.

(Well, unless they implement some custom type that defines as_slice, but I don't think anyone actually wants to do that)

[nfiles]

No problem!

After this is merged, are there any remaining instances of unwrap that should be changed?

I think the remaining unwrap calls are okay, but I could certainly be wrong. I'm still pretty new to Rust. None of them are being directly used in a test assertion. If the worry is that the unwrap call can cause a panic at any point in the test (not just the assertion), it might make sense to assert!(wrapped.is_ok()) before calling unwrap. I'm not sure if that makes sense.

unwrap is still called the most in queen-attack, react, and triangle, but none of them are within a test assertion.

1. queen-attack - ChessPosition::new is unwrapped and passed directly into Queen::new.
2. react - reactor.create_compute is unwrapped because it is passed into other methods to set up the test assertions.
3. triangle - Triangle::build is unwrapped before the test assertions. An is_ok and is_err result are both tested.
4. circular-buffer - buffer.read().unwrap().len() is called within the test assertion, but the unwrapped value is not being directly compared, so I thought it was okay.
5. nucleotide-count - dna::nucleotide_counts(s) is unwrapped, but the unwrapped HashMap is tested with a couple of different iterators, and I don't know how to replace that with a test assertion on a wrapped object.

That's a good point about the as_slice/vec! instances (all-your-base and variable-length-quantity). The example.rs file was my reference for those and they explicitly returned Vec in the optional type.

This pull request resolves #338

[petertseng]

Thanks again.

When I am able, I will also check to see whether I agree with your assessment, and assuming I do then it will be time to merge it.

This pull request resolves #338

That needs to be in the PR description (the one that currently says "Related to Issue #338") OR any commit message in the PR.

[nfiles]

Sorry, I understand now. I have updated the PR description.

[petertseng]

OK. I agree with all your assessments. And they are definitely not unwraps that are compared, so objective achieved. I may consider a new issue for providing better error messages when the unwraps fail, if there is demand for it (or someone else may open it, that will be sufficient demand). As for this PR, it is time to merge, and close the referenced issue.

I will be making a change to the commit messages. They should be written in imperative form to match with the messages that git emits (e.g. on git merge and git revert), so at a minimum the "Changes" should be "Change"

[petertseng]

Thanks (yet) again, great stuff.