Remove unwrap() in receive::v1::test
#543
Conversation
There was a problem hiding this comment.
Thanks for taking a shot at this for the first time with minimal instruction.
The general approach is correct but we should be able to refine it so that the changes to the tests are a net removal. (edit: impl StdError might add lines, but that's OK. I mean changes specifically to test lines since we're removing .unwrap() lines.)
Anywhere there's a .map_err(|e| e.to_string())?; or map_err(|e| format!(...*in tests* should be replaced by a simple?in tests unless there's a very good reason to do otherwise. If std::error::Error needs to be implemented onInternal*` error types in order to make this happen, that should be done. If StdError is implemented on internal types, that should be used chained to replace the non-internal StdError implementation to de-duplicate work.
Pull Request Test Coverage Report for Build 13384843632Details
💛 - Coveralls |
| .unwrap() | ||
| .require_network(network) | ||
| .unwrap()) | ||
| // Convert script to address with proper error handling |
There was a problem hiding this comment.
Inline comments like this tend to go stale, so I prefer not to use them except if overwhelmingly necessary, instead abstracting to a helper function with a /// docstring. I don't think any comment is necessary here.
| assert!(psbt.is_ok(), "Payjoin should be a valid PSBT"); | ||
| payjoin | ||
| .apply_fee(None, None) | ||
| .map_err(|e| format!("Payjoin should be a valid PSBT: {:?}", e))?; |
There was a problem hiding this comment.
In e.g. payjoin-cli adding context is helpful. In the tests where you get a line number and you're a dev running tests and reading code, I'd rather the error is propagated with a simple ?, even if that requires impl StdError on inner types. That way we can reduce noise in LoC and custom static strings in the tests.
| .require_network(network) | ||
| .unwrap()) | ||
| let script_address = Address::from_script(script, network) | ||
| .map_err(|e| e.to_string())?; |
There was a problem hiding this comment.
Prefer a simple ? wherever there isn't an overwhelming reason not to use one e.g. if context is necessary in application code
| let mut payjoin_clone = payjoin.clone(); | ||
| let psbt = payjoin.apply_fee(None, Some(FeeRate::from_sat_per_vb_unchecked(1000))); | ||
| assert!(psbt.is_ok(), "Payjoin should be a valid PSBT"); | ||
|
|
||
| let psbt = payjoin_clone.apply_fee(None, Some(FeeRate::from_sat_per_vb_unchecked(995))); | ||
| assert!(psbt.is_err(), "Payjoin exceeds receiver fee preference and should error"); | ||
|
|
||
| Ok(()) |
There was a problem hiding this comment.
I sincerely appreciate the change and offer my feedback in hopes it helps make contributions without friction based on the customs of this repo while recognizing you woudn't know to do this without prior instruction.
Make sure to read all of the Contributing section of the README, and specifically apply the formatting and linting sections in order to avoid CI failures from new indentation like this. cargo fmt on nightly would prevent this from a new indentation change.
Also, I see a space was added on 907. Don't add new whitespace outside of a content or formatting commit. I think this change should really just be the 2 lines (whitespace and Ok((())
There was a problem hiding this comment.
Totally understand. Thanks for flagging that.
I checked the contribution section for testing but I did indeed miss the part on formatting and lint. I'll work on addressing them. Thx!
Partially address #487
Replace
.unwrap()with.map_errwherever applicable. Otherwise fallback to?