feat: add Value3, V3, Value4, V4 #14
Conversation
|
|
||
| fmt.Println("Panic message:", must.Panic(func() { | ||
| must.Value2(1, 2, errors.New("P")) | ||
| }).(error).Error()) |
There was a problem hiding this comment.
I think we should write one good example for all Value# functions and write the rest as tests. What do you think?
There was a problem hiding this comment.
I think we should merge to have good coverage, and then work on doc improvements.
Doc and examples for Value3/V3/Value4/V4 are quite useless because redundant (if someone has read Value2/V2 that should be enough), but we could improve examples by using concrete functions from stdlib.
There was a problem hiding this comment.
The coding style (error type assertion) is fine as a test, but should not be in an example because it's not guaranteed by the API https://github.com/xiegeo/must#compatibility
Values used in panics are only for debugging. No guarantees are provided on how panics are constructed.
If you have a use case. It's possible to provide a function or change the docs to reflect it. My thinking on this is if you need panic to be created in a particular way, it's better to roll your own than use a dependency.
There was a problem hiding this comment.
The assertion is applied on the return value from must.Panic. In this case it is an error because must.Value2 panics with the errors.New("P") result.
I don't think that my example goes beyond the API contract. But could you reply to those questions:
- Isn't it guaranteed that
must.Value2panics with the 3 argument? - Isn't it guaranteed that
must.Panicreturns the raw value caught by recover which, in this case, is anerrorbecause it comes from the 3 argument given toValue2?
There was a problem hiding this comment.
Well the response to the second question is in https://github.com/xiegeo/must#compatibility and was written in #14 (comment).
There was a problem hiding this comment.
Would it be ok if I transform the Examples into just Tests?
Add Value3, V3, Value4, V4. Add test coverage for Value2.
dolmen
force-pushed
the
feat/Value3,Value4
branch
from
e443440 to
1c30cb7
Compare
Add
Value3,V3,Value4,V4likeValue,Value2.Add test coverage for
Value2.