-
Notifications
You must be signed in to change notification settings - Fork 13
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
Implement kaitai.Struct common interface #30
Merged
Merged
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
cac6536
Implement kaitai.Struct: common interface mandating Kaitai_IO() metho…
GreyCat cfc42b9
Make test less obscure as per PR discussion
GreyCat 9f594fd
Fix lint problems, add some explanations in the docstrings
GreyCat af7c638
Renamed `Kaitai_IO` -> `IO_` as per PR discussion
GreyCat File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| package kaitai | ||
|
|
||
| // Struct is the common interface guaranteed to be implemented by all types generated | ||
| // by Kaitai Struct compiler. | ||
| type Struct interface { | ||
| Kaitai_IO() *Stream | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| package kaitai | ||
|
|
||
| import ( | ||
| "bytes" | ||
| "testing" | ||
| "github.com/stretchr/testify/assert" | ||
| ) | ||
|
|
||
| // Declare two struct simulating types generated by Kaitai Struct compiler | ||
| type oneStruct struct { | ||
| one int | ||
| _io *Stream | ||
| } | ||
|
|
||
| func (s *oneStruct) Kaitai_IO() *Stream { | ||
| return s._io | ||
| } | ||
|
|
||
| type twoStruct struct { | ||
| two int | ||
| _io *Stream | ||
| } | ||
|
|
||
| func (s *twoStruct) Kaitai_IO() *Stream { | ||
| return s._io | ||
| } | ||
|
|
||
| func WorkWithStruct(s Struct, t *testing.T, expectedSize int) { | ||
| actualSize, err := s.Kaitai_IO().Size() | ||
| assert.Equal(t, actualSize, int64(expectedSize)) | ||
GreyCat marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| assert.Nil(t, err) | ||
GreyCat marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| func TestKaitaiStruct(t *testing.T) { | ||
| // Instantiate streams for the structs | ||
| oneStream := NewStream(bytes.NewReader([]byte("a"))) | ||
| twoStream := NewStream(bytes.NewReader([]byte("ab"))) | ||
|
|
||
| // Instantiate the structs | ||
| one := oneStruct{1, oneStream} | ||
| two := twoStruct{2, twoStream} | ||
|
|
||
| // Check if the structs implement the Struct interface | ||
| WorkWithStruct(&one, t, 1) | ||
| WorkWithStruct(&two, t, 2) | ||
GreyCat marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Hmmm. I think this is fairly unideal, though it's not obvious how one could do better without changing the attribute name mangling.
Other potential bikeshed paint colors:
IO_(),KaitaiIO_()Though maybe special casing
kaitai_i_oin the compiler is not a bad idea. It's not like this is likely to come up in non-contrived examples anyways...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 is my new favorite. I knew that if you start the identifier with an underscore
_, then it doesn't start with a capital letter and won't be exported, but I didn't think of ending the identifier with an underscore_. It's simple and I suppose it potentially works better with autocompletion (assuming that only identifiers starting with what you type are shown) - in case a user would sometimes need to refer to_io,_parentor_root, they can start typingI,ParorRoand will be offeredIO_(),Parent_()andRoot_()respectively, and they don't have to remember that there is a specialKaitai_prefix.This may be true in practice, but I still advocate that there should be no "holes" in the language. There are already some holes in our Go implementation that I'm not happy about (e.g. the
Readmethod -hello_world.go:18, which will clash with a user-specified instance calledread), but at least I'd avoid adding more.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.
Thanks! I also like the idea of
IO_(). Just to get an Go expert opinion: how bad it will be from ecosystem perspective — e.g. it will generate code that will have to break varnames linting? How serious it is?On the side note, to be honest, I won't mind breaking compat and turning Go
Readmethod intoRead_for example (given that vast majority of other languages generates_read()or some similar).