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

Custom Scan enhancement #538 #166

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Custom Scan enhancement #538 #166

wants to merge 2 commits into from

Conversation

rodmartin30
Copy link

Adding first things for custom scan and simulating terminator byte.

@rodmartin30
Copy link
Author

Talking with @GreyCat he noticed me some things:

I've taken a look, so on a broader scale:
I won't touch BytesEosType and BytesTerminatedType — I'd say that there's very little value in having both size + scan-end support at once, or terminator + scan-end
So, BytesScanEndType alone would probably more than enough.

Should I extend BytesType with ScanEnd? or maybe it would be better do something like this:

case class BytesScanEndType(
    override val process: Option[ProcessExpr],
    override val scanEnd: Option[ScanExpr] = None
  ) extends BytesType with ScanEnd

Also, for ScanExpr, it's probably just enough to have ScanCustom arguments right in the ScanExpr, and avoid doing that "trait inherited by multiple classes" thing, and then switching it over every time like this: https://github.com/kaitai-io/kaitai_struct_compiler/compare/538#diff-121a1b8f73b9f24552fa03a24f589eedR191
you can basically remove ScanExpr, replace every time it's mentioned with ScanCustom, and rename ScanCustom to ScanExpr

@GreyCat
Copy link
Member

GreyCat commented Jun 5, 2019

case class BytesScanEndType(
    override val process: Option[ProcessExpr],
    override val scanEnd: Option[ScanExpr] = None
  ) extends BytesType with ScanEnd

Yep, this looks like the best solution: there's no need to add that functionality to every bytes type, a dedicated feature to will be the best.

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