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

LazyStruct and LazySequence don't use kwargs #365

Closed
movermeyer opened this Issue Jun 6, 2017 · 7 comments

Comments

Projects
None yet
2 participants
@movermeyer

movermeyer commented Jun 6, 2017

When using the Python 3.6+ keyword argument syntax to build constructs, building lazy constructs fails.

Example Code:

from construct import Struct, LazyStruct, Byte

MY_STRUCT_NON_KW = Struct(
    "a" / Byte,
    "b" / Byte,
    "c" / Byte,
    "d" / Byte,
)

MY_LAZY_STRUCT_NON_KW = LazyStruct(
    "a" / Byte,
    "b" / Byte,
    "c" / Byte,
    "d" / Byte,
)

MY_STRUCT = Struct(a=Byte, b=Byte, c=Byte, d=Byte)
MY_LAZY_STRUCT = LazyStruct(a=Byte, b=Byte, c=Byte, d=Byte)

print("MY_STRUCT_NON_KW:", MY_STRUCT_NON_KW.parse(b'\x00\x00\x00\x00'))
print("MY_LAZY_STRUCT_NON_KW:", MY_LAZY_STRUCT_NON_KW.parse(b'\x00\x00\x00\x00'))
print("MY_STRUCT:", MY_STRUCT.parse(b'\x00\x00\x00\x00'))
print("MY_LAZY_STRUCT:", MY_LAZY_STRUCT.parse(b'\x00\x00\x00\x00'))

gives:

MY_STRUCT_NON_KW: Container:
    a = 0
    b = 0
    c = 0
    d = 0
MY_LAZY_STRUCT_NON_KW: <LazyContainer: 4 possible items, 0 cached>
MY_STRUCT: Container:
    a = 0
    b = 0
    c = 0
    d = 0
MY_LAZY_STRUCT: <LazyContainer: 0 possible items, 0 cached>

I would have expected the lazy versions to produce the same values as the non-lazy versions.
Especially since the docs say they are equivalent and take in kwargs:

construct.LazyStruct(*subcons, **kw):
Equivalent to Struct construct

This is a result of LazyStruct and LazySequence not using their kw parameters at all.

Credits: Discovered with help from pylint

@arekbulski arekbulski added the bug found label Jun 6, 2017

@arekbulski

This comment has been minimized.

Collaborator

arekbulski commented Jun 6, 2017

Good find! Do you feel up to the challange to fix it? I trust your code.
Proper test case is also needed. You would need to turn prints into asserts.

@arekbulski arekbulski changed the title from `LazyStruct` and `LazySequence` don't use their kwargs to LazyStruct and LazySequence don't use kwargs Jun 7, 2017

@arekbulski

This comment has been minimized.

Collaborator

arekbulski commented Jun 7, 2017

I assume you found this in connection to previous topic #359

@movermeyer

This comment has been minimized.

movermeyer commented Jun 7, 2017

No, actually.

As part of my editor (Atom or VSCode), I run several static analysis tool/linters (pylint, flake8, autopep8) as plugins.

When I opened the construct code in my editor, the linters raised some interesting warnings, including this one: "Unused argument kw". I then investigated and found that it was actually a bug and reported it.

Most of the credit belongs to pylint.

@movermeyer

This comment has been minimized.

movermeyer commented Jun 7, 2017

As for this bug, I'll try to fix it.

@movermeyer

This comment has been minimized.

movermeyer commented Jun 7, 2017

I've fixed LazyStruct (very easy, see #368), but LazySequence is a more in-depth change.

Can I leave that to you?

@arekbulski

This comment has been minimized.

Collaborator

arekbulski commented Jun 7, 2017

Of course, thank you for all effort so far.

@arekbulski

This comment has been minimized.

Collaborator

arekbulski commented Jun 16, 2017

Fixed.

@arekbulski arekbulski closed this Jun 16, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment