add uint16 support #217
add uint16 support #217
Conversation
…nto ceyonur/hexutil-uint16
There was a problem hiding this comment.
and thus cannot be generated through
github.com/fjl/gencodec.
This premise is incorrect, as demonstrated by 016fd90. gencodec can indeed support raw uint16 and it will result in decimal output. As the intended use case is for milliseconds, for which hex encoding would be unusual (or even irritating), I think this PR should be closed.
I've done a quick once-over review in case there's a strong argument to the contrary that I've missed as it will at least allow you to continue with the fixes. If not, then feel free to ignore the individual code comments.
There was a problem hiding this comment.
If the motivation is to support JSON marshalling then these functions are unnecessary.
| if err != nil { | ||
| err = mapError(err) | ||
| if err == ErrUint64Range { | ||
| return 0, ErrUint16Range | ||
| } | ||
| } | ||
| return uint16(dec), err //nolint:gosec // G115 won't overflow uint16 as ParseUint uses 16 bits |
There was a problem hiding this comment.
| if err != nil { | |
| err = mapError(err) | |
| if err == ErrUint64Range { | |
| return 0, ErrUint16Range | |
| } | |
| } | |
| return uint16(dec), err //nolint:gosec // G115 won't overflow uint16 as ParseUint uses 16 bits | |
| if err != nil { | |
| err = mapError(err) | |
| if err == ErrUint64Range { | |
| err = ErrUint16Range | |
| } | |
| return 0, err | |
| } | |
| return uint16(dec), nil //nolint:gosec // G115 won't overflow uint16 as ParseUint uses 16 bits |
| import "testing" | ||
|
|
||
| var ( | ||
| encodeUint16Tests = []marshalTest{ |
There was a problem hiding this comment.
Test tables should be in the test function, not separated out like this. It adds unnecessary distance for the reader, who can't simply check the test loop and its inputs at the same time.
| input, ok := test.input.(uint16) | ||
| if !ok { | ||
| t.Errorf("input %v: not a uint16", test.input) | ||
| } |
There was a problem hiding this comment.
This is a code smell suggesting that the marshalTest struct type isn't fit for purpose. The only benefit to using it is saving 2 lines in declaring the fields, while the cost is this extra check as well as having to explicitly wrap each input in uint(...).
| if !ok { | ||
| t.Errorf("input %v: not a uint16", test.input) | ||
| } | ||
| enc := EncodeUint16(input) |
There was a problem hiding this comment.
| enc := EncodeUint16(input) | |
| got := EncodeUint16(input) |
The value under test should be named got, unless there is a need for disambiguation between multiple values, in which case use a suffix like gotFoo.
| if !isString(input) { | ||
| return errNonString(uint16T) | ||
| } | ||
| return wrapTypeError(b.UnmarshalText(input[1:len(input)-1]), uint16T) |
There was a problem hiding this comment.
(subjective) I would explicitly check the error and only call the wrapTypeError function if non-nil, even though it checks for this. The first time I read the code I was really confused by why there was always an error being returned.
| raw, err := checkNumberText(input) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if len(raw) > 4 { | ||
| return ErrUint16Range | ||
| } | ||
| var dec uint16 | ||
| for _, byte := range raw { | ||
| nib := decodeNibble(byte) | ||
| if nib == badNibble { | ||
| return ErrSyntax | ||
| } | ||
| dec *= 16 | ||
| dec += uint16(nib) //nolint:gosec // G115 won't overflow uint16 as decodeNibble uses 4 bits | ||
| } | ||
|
|
||
| *b = Uint16(dec) | ||
| return nil |
There was a problem hiding this comment.
As above re reusing DecodeUint64 if it were to be kept. If not, then strconv.ParseUint should be used in place of the nibble decoding.
|
|
||
| func TestUnmarshalUint16(t *testing.T) { | ||
| for _, test := range unmarshalUint16Tests { | ||
| var v Uint16 |
There was a problem hiding this comment.
| var v Uint16 | |
| var got Uint16 |
There was a problem hiding this comment.
Most comments on hexutil.libevm_test.go apply to this whole file too.
| t.Errorf("%d: %v", test.input, err) | ||
| continue | ||
| } | ||
| if want := `"` + test.want + `"`; string(out) != want { |
There was a problem hiding this comment.
| if want := `"` + test.want + `"`; string(out) != want { | |
| if want := strconv.Quote(test.want); string(got) != want { |
Intent is explicit.
|
Closing this as it seems we either not need this for a |
3 participants
Why this should be merged
ACP-226 proposes adding
timeMilisecondsas auint16field. However it's not supported in thehexutilJSON encoder and thus cannot be generated throughgithub.com/fjl/gencodec.This PR intends to add that required support
How this works
Adds
uint16types to json and hexutilHow this was tested
Added relevant tests