-
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
Conversation
…d on all ksc-generated types
9ce0be2
to
cac6536
Compare
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.
I don't think we need to add a (relatively heavy) dependency github.com/stretchr/testify
just to make literally 2 lines of test code slightly easier to write, but if you think it's not a problem, then it's not a problem.
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.
Threw in my 2c. I don't have strong opinions but the general idea seems fine.
kaitai/struct.go
Outdated
// This is deliberately named with a `Kaitai_` prefix and underscore to avoid conflicts | ||
// with other methods that may result from the attributes in Kaitai Struct type, e.g. | ||
// is a user will define attribute `i_o` this will conflict with the method `IO()`. | ||
Kaitai_IO() *Stream |
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_o
in 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.
IO_()
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
, _parent
or _root
, they can start typing I
, Par
or Ro
and will be offered IO_()
, Parent_()
and Root_()
respectively, and they don't have to remember that there is a special Kaitai_
prefix.
Though maybe special casing
kaitai_i_o
in the compiler is not a bad idea. It's not like this is likely to come up in non-contrived examples anyways...
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 Read
method - hello_world.go:18
, which will clash with a user-specified instance called read
), 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 Read
method into Read_
for example (given that vast majority of other languages generates _read()
or some similar).
52a987c
to
af7c638
Compare
The
This seems to be a common problem that many users of https://github.com/golangci/golangci-lint-action have run into, but I'm not 100% sure what's causing it and how to fix it. Apparently, it's a problem with restoring the cache done by Despite reading a decent portion of the related issues (but definitely not all, since there are many of them), I don't have a definitive answer. In this issue - golangci/golangci-lint-action#863 (comment) - it was suggested that the caching done by It's possible that the kaitai_struct_go_runtime/.github/workflows/go.yml Lines 38 to 41 in e822da7
... but I don't know. I only added it because I saw it in golangci/golangci-lint-action#902 (comment) and it reportedly fixed certain problems (but now after taking a closer look, they actually enabled the caching on Since all the offending files seem to have paths like with:
# ...
version: v1.54
# ...
# Optional: if set to true, then the action won't cache or restore ~/go/pkg.
# skip-pkg-cache: true ... but I'm still curious whether it's somehow our fault, or indeed a bug in the |
@generalmimon That's a very fair concern around lint, and indeed I agree it looks weird. Even if we're doing anything wrong, I would welcome golangci-lint-action/ to have some preventive diagnostics instead of just blindly dumping 3000+ errors. Thanks for your thorough investigation :) Indeed, let's give a try with |
From our CI runs that we've seen so far, it only happens in pull requests - I haven't seen it in any of the runs on Of all the runs on this PR, the first run doesn't have the problem: Step Run golangci-lint
Step Post Run golangci-lint
... but the second run and all the others have the problem: Step Run golangci-lint
Step Post Run golangci-lint
|
@GreyCat FYI, there has been a new major version
The PR message of golangci/golangci-lint-action#1024 claims to fix issues like golangci/golangci-lint-action#863 and golangci/golangci-lint-action#807, which both refer to the same So hopefully we can resolve the cache-related problems by updating to |
Analogous to KaitaiStruct in all other languages (like Java), this adds
kaitai.Struct
interface, mandating Kaitai_IO() method on all ksc-generated types in go.