poker: also using Result for the tested function#970
poker: also using Result for the tested function#970elektronaut0815 wants to merge 2 commits intoexercism:masterfrom
Conversation
coriolinus
left a comment
There was a problem hiding this comment.
Thank you for doing this work, but I do not intend to merge this PR unless you can persuade me to change my mind. Rationale:
-
Option<Vec<_>>is already questionably idiomatic; an empty vector conveys precisely the same information, namely that there are no winning hands provided.Result<Vec<_>, _>doesn't provide any more information than that, because there's still only one circumstance that would trigger theErr(_)case; it's just more verbose. -
Result<_, &'static str>is an antipattern: if there's only one way a function can fail, it's usually better to return anOption, and if there are multiple, thenpub enum Erroris more powerful and flexible. It's fine in things like the example, which most students don't see, but we should absolutely not put that into the public interface that every student must implement. -
assert!(whatever().is_err());is an antipattern: it's always better to useassert_eq!(whatever().unwrap_err(), Error::ExpectedError);(
assert!(whatever().is_none());is fine, though, because we're not throwing away information about which error has been raised in this case.)
I will basically always approve PRs which only add new tests to the suite, so if you wish to update the new cases to the existing API and discard the rest of the changes, we can get this merged. Otherwise, recommend closing this PR. If there is no further action, then I will close this at some point later than 1 week from now.
|
Thank you for the comment. Sounds reasonable and is good advice for me on my Rust journey. |
|
Following up on the antipattern discussion:
|
I think it's more consistent if Result is used everywhere.