-
Notifications
You must be signed in to change notification settings - Fork 807
Add stringer to warp types #2712
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of the other functions are defined on the pointers... I think these should be too. Aside from that lgtm
This was intentional since the string function doesn't modify the values, so there's no need for them to take a pointer receiver. Also, the Lmk if you still prefer changing these functions to use the pointer receiver and I'll update it and the reference in Message's |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was intentional since the string function doesn't modify the values, so there's no need for them to take a pointer receiver. Also, the
Message
type has anUnsignedMessage
value field instead of a pointer, so the alternative was to take a reference to that in itsString()
function, which is fine, I just slightly preferred defining the function on the value.Lmk if you still prefer changing these functions to use the pointer receiver and I'll update it and the reference in Message's
String()
function.
The main 2 reasons I feel like it should be on the pointer type are:
- Users are normally going to hold the pointer type (because we expect/return a pointer to these structs everywhere we expose them)
NewUnsignedMessage
ParseUnsignedMessage
NewMessage
ParseMessage
All take in/return pointers ^
- Consistency
I'd like them to be pointer receivers unless you feel strongly against it.
Yup, just updated |
Why this should be merged
This PR implements
fmt.Stringer
on each of the warp message/signature types includingMessage
,UnsignedMessage
,BitSetSignature
, and theSignature
interface.How this works
This adds a
String()
function to each of the interfaces following the<type>(key1 = val1, key2 = val2, ...)
string format from the consensus instance stringer functions (with the exception ofMessage
which omits the keys since both the UnsignedMessage and Signature already include their own types).How this was tested
Tested with one-off unit tests to see the output (not worth including in source imo):