Update select_first_candidate to return InternalSelectionError::Empty
#542
Conversation
|
Hi there! I'm new here. I didn't see this covered in any existing tests, unless you'd like me to add one? |
|
If you are up to adding a test that'd be a welcome addition |
Pull Request Test Coverage Report for Build 13667156981Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
|
Also, currently we actually check for an empty candidates set twice: the first one is here. So this specific code change would be unreachable unless calling |
|
Yes please remove the empty check in |
pseudoramdom
force-pushed
the
535-select_first_candidate-error
branch
from
c113c09 to
57d21c0
Compare
|
I made the requested changes. |
|
@pseudoramdom Do you mind squashing your commits into one? |
pseudoramdom
force-pushed
the
535-select_first_candidate-error
branch
from
c5aa1ea to
5066eef
Compare
There was a problem hiding this comment.
ACK 5066eef
Thanks @pseudoramdom and reviewers!
nit: looks like all previous commit messages got squashed into this one, it would be nice to clean up the commit message before merging this. We generally follow the commit guidelines established in https://cbea.ms/git-commit/
There was a problem hiding this comment.
LGTM other than the commit messages. Please follow the seven rules from cbeams.
Thank you for your contribution 😎
…pty` Fix payjoin#535 - Update select_first_candidate to return InternalSelectionError::Empty when there are no candidates available for selection. - Include test `empty_candidates_inputs`
pseudoramdom
force-pushed
the
535-select_first_candidate-error
branch
from
5066eef to
25d2af5
Compare
|
Thanks for the reviews folks! Fixed the commit message. Let me know if it looks good. |
There was a problem hiding this comment.
Thanks for the update. I think there is one sincere issue with the test, as discovered by @elnosh in another PR that I've repeated in my comments below.
I would like to understand the rationale of the change in the PR body, in complete sentences, rather than just a repeat of the title. The commit message is closer but I still need it to follow the seven rules, namely 2, 6, and 7, seem broken to me. Much more important than the what is the why, or rationale for the change.
| if let Err(err) = result { | ||
| let debug_str = format!("{:?}", err); | ||
| assert!( | ||
| debug_str.contains("Empty"), | ||
| "Error should indicate 'Empty' but was: {}", | ||
| debug_str | ||
| ); | ||
| } | ||
| Ok(()) |
There was a problem hiding this comment.
This pattern will ignore an Ok result where there should be an error. Instead match, panic! on the Ok case and assert! on the Err case as pointed out by @elnosh
| @@ -847,6 +845,42 @@ pub(crate) mod test { | |||
| } | |||
| } | |||
|
|
|||
| #[test] | |||
| fn empty_candidates_inputs() -> Result<(), BoxError> { | |||
There was a problem hiding this comment.
You can take advantage of the BoxError here by using ? to propagate errors instead of calling .expect. Doing so will make the test more brief and readable.
A followup commit might make this change to use BoxError in the other tests in this module.
Fix #535
Update
select_first_candidateto returnInternalSelectionError::Emptywhen there are no candidates available for selection