Implement kaitai.Struct common interface #30
Conversation
…d on all ksc-generated types
GreyCat
force-pushed
the
add_kaitai_struct
branch
from
9ce0be2
to
cac6536
Compare
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.
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.
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_oin 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.
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).
GreyCat
force-pushed
the
add_kaitai_struct
branch
from
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.Structinterface, mandating Kaitai_IO() method on all ksc-generated types in go.