Skip to content
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

support uint16/32, and tests added #15

Merged
merged 6 commits into from
Jul 26, 2024
Merged

support uint16/32, and tests added #15

merged 6 commits into from
Jul 26, 2024

Conversation

Lavishq
Copy link
Contributor

@Lavishq Lavishq commented Jul 26, 2024

#12

image
image

  • Added uint16 support
  • Added uint32 support
  • tests

@karalabe
Copy link
Owner

karalabe commented Jul 26, 2024

Sweet, very nice :)

One more thing to make it perfect if you're up for it: tests :D But don't worry, it's easy because the test data is there, it just needs to be called:

If you're feeling a bit more adventurous, you can also add VarTestStruct and ComplexTestStruct, which isn't much harder, just the fields types are not only primitives any more, rather you'll need to dig a bit deeper. But if you don't want to, I'm happy with the first two types only too :)

@karalabe karalabe added this to the 0.3.0 milestone Jul 26, 2024
Copy link

codecov bot commented Jul 26, 2024

Codecov Report

Attention: Patch coverage is 81.44330% with 18 lines in your changes missing coverage. Please review.

Project coverage is 83.85%. Comparing base (be92ed9) to head (ba211b5).

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #15      +/-   ##
==========================================
- Coverage   84.00%   83.85%   -0.15%     
==========================================
  Files          59       61       +2     
  Lines        2938     3035      +97     
==========================================
+ Hits         2468     2545      +77     
- Misses        313      324      +11     
- Partials      157      166       +9     
Components Coverage Δ
ssz 83.11% <79.41%> (-0.31%) ⬇️
sszgen 70.99% <77.77%> (+0.15%) ⬆️

@Lavishq
Copy link
Contributor Author

Lavishq commented Jul 26, 2024

did the tests and able to go generate w/o errors for
SmallTestStruct and FixedTestStruct here

where as having issues getting complex dynamic dtypes work, since idk golang

type VarTestStruct struct {
	A [1]byte `ssz-size:"2" ssz:"bits"`
	// B [1]byte `ssz-size:"2" ssz:"bits" ssz-max:"1024"`
	C [1]byte `ssz-size:"1" ssz:"bits"`
}

type ComplexTestStruct struct {
	A [1]byte `ssz-size:"2" ssz:"bits"`
	// B [1]byte `ssz-size:"2" ssz:"bits" ssz-max:"128"`
	C [1]byte `ssz-size:"1" ssz:"bits"`
	// D [1]byte `ssz-max:"256"`
	E VarTestStruct
	// F [1]FixedTestStruct `ssz-size:"4" ssz:"bits"`
	// G [1]VarTestStruct `ssz-size:"2" ssz:"bits"`
}

have reached til^

sorry, if im wrong w/ things since this is the first time im programming in go-
so learning while trying this :)`

@Lavishq Lavishq changed the title support uint16/32 added support uint16/32, SmallTestStruct && FixedTestStruct added Jul 26, 2024
@Lavishq Lavishq changed the title support uint16/32, SmallTestStruct && FixedTestStruct added support uint16/32, and tests added Jul 26, 2024
@karalabe karalabe mentioned this pull request Jul 26, 2024
codec.go Outdated Show resolved Hide resolved
cmd/sszgen/opset.go Outdated Show resolved Hide resolved
cmd/sszgen/opset.go Outdated Show resolved Hide resolved
Copy link
Owner

@karalabe karalabe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All tests green. Thank you for the PR. LGTM! :)

@karalabe karalabe merged commit 026e397 into karalabe:main Jul 26, 2024
4 checks passed
@karalabe
Copy link
Owner

@Lavishq Just one more tip in general, in Go, you can run all the tests from all packages via go test ./.... The one you did running go test only ran it for tests in the root folder, which aren't really many/any. This is generally not the case for Go packages of course, but for this specific project there is such an extensive external test suite, that it made little sense to redo internal unit tests when I could just rely on the external suite.

Thanks again :)

@Lavishq
Copy link
Contributor Author

Lavishq commented Jul 26, 2024

hm, i know nothing about go, i once did tour of go upto variables and imports, probably a little more for about 20-30 mins and that is all [ also dont remember much ]
so i do need to learn it the right way, recently was looking at op-geth and didnt understand much, thought that since it is gfg i would quickly learn from gobyexample or the codebase itself ^^

thanks ser, for bearing with me, it was embarrasing to make silly mistakes but was a learning exp and would do again but maybe after i finish tour of go this time :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants