Pull Request Test Coverage Report for Build 12223807707

Details

• 10 of 12 (83.33%) changed or added relevant lines in 2 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-0.002%) to 82.872%

Totals
Change from base Build 12188753888:	-0.002%
Covered Lines:	20201
Relevant Lines:	24376

💛 - Coveralls

What is the benefit of panicing instead of returning an error?

What is the benefit of panicing instead of returning an error?

Are you asking what's the benefit of having an unchecked variant?

• If you want to do something like from_sats(42) and you don't want to bother handling an error because you know by looking at the call that it won't panic or error.
• It makes the upgrade path simple because we already provide an unchecked variant from_sats.

I wonder though if it would be wiser to deprecate from_sats and make a new method with the panic behavior called from_sats_unwrap to explicitly opt in to behavior change. I had named it from_sats_unchecked in a previous PR but its doing a check and then panic so that name doesn't make sense I think.

What is the benefit of panicing instead of returning an error?

Are you asking what's the benefit of having an unchecked variant?

I was referring to this line: "Explicitly panic in function from_sats"

The question stands.

I was referring to this line: "Explicitly panic in function from_sats"

@tcharding I'll try to be more precise with what was meant by "Explicitly panic in function from_sats" to hopefully dispel any confusion. What is meant is: Explicitly panic if the value passed exceeds max. In otherwords, don't overflow and panic no matter which mode.

As for your earlier question "What is the benefit of panicing instead of returning an error?". The benefit of panicking is that you would only call this function ideally when you either don't care if it panics or if you know it won't. That way, you don't need to worry about handling the error case upon return. Also, there are a number of functions in the test suit that do not expect an error to be returned, and it would be a rather large and disruptive change to add that now, not to mention for those who depend on this downstream.

Lastly, there is also the checked version from_sats_checked for the case where you want to know if max is exceeded and gracefully handle the functions return. I return an Option instead of Result in that case since it seems consistent with other functions in this library to do so. Although, I'm open to suggestions if it seems like I'm overlooking something.

Lastly, there is also the checked version from_sats_checked for the case where you want to know if max is exceeded

Cool, we got to the bottom of it. IMO from_sats should return an error and there should be not be a function called from_sats_checked

Cool, we got to the bottom of it. IMO from_sats should return an error and there should be not be a function called from_sats_checked

@tcharding I mean I guess from_sats could return an error, and then we don't need from_sats_checked. This is counter to your comment on #3692 I think:

The unwraps are ugly, perhaps we should add a from_sat_unchecked function that doesn't enforce the invariant?

If we change this to return a result, there's going to be a lot of ugly unwraps(), right.

Fuck, you are using up a lot of my time these days @yancyribbens

This is counter to your comment on #3692 I think:

Incorrect.

Fuck, you are using up a lot of my time these days @yancyribbens

That's because I contribute a lot to this project, more than anyone else except yourself and @apoelstra, so I take this as a compliment.

closing in favor of #3730