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

Error message says 'bytes', not bits when there aren't enough bits for BitsInteger #360

Closed
movermeyer opened this issue Jun 4, 2017 · 8 comments
Labels

Comments

@movermeyer
Copy link

@movermeyer movermeyer commented Jun 4, 2017

If you try to read x more bits than are available using a BitsInteger, the error message incorrectly states that you are trying to request x extra bytes, not bits.

Example code:

from construct import BitStruct, Bit, BitsInteger

MY_MESSAGE = BitStruct(
    Bit[7],
    BitsInteger(17)
)

MY_MESSAGE.parse(b'A')
Traceback (most recent call last):
  File "test.py", line 8, in <module>
    MY_MESSAGE.parse(b'A')
  File "env\lib\site-packages\construct\core.py", line 165, in parse
    return self.parse_stream(BytesIO(data), context, **kw)
  File "env\lib\site-packages\construct\core.py", line 176, in parse_stream
    return self._parse(stream, context, "parsing")
  File "env\lib\site-packages\construct\core.py", line 1791, in _parse
    obj = self.subcon._parse(self.stream2, context, path)
  File "env\lib\site-packages\construct\core.py", line 849, in _parse
    subobj = sc._parse(stream, context, path)
  File "env\lib\site-packages\construct\core.py", line 576, in _parse
    data = _read_stream(stream, length)
  File "env\lib\site-packages\construct\core.py", line 72, in _read_stream
    data = stream.read(length)
  File "env\lib\site-packages\construct\lib\bitstream.py", line 26, in read
    raise IOError("Restreamed cannot satisfy read request of %d bytes" % count)
OSError: Restreamed cannot satisfy read request of 17 bytes

While trying to parse a complicated object, I was confused as to what combination of my bit fields added to the number of bytes presented in the error message.

In the above example, it should report OSError: Restreamed cannot satisfy read request of 17 bits

@arekbulski arekbulski added the question label Jun 4, 2017
@arekbulski
Copy link
Member

@arekbulski arekbulski commented Jun 4, 2017

That is not entirely a bug. You see, some classes are kind of agnostic with bits and bytes, so to make the story short, its just all called bytes. BitStream which is the class raising the exception does NOT know whether there are bits or bytes that are being processed. Despite the name of the class. Its a universal class that transforms bits->bytes and bytes->bits but can also do other transformations.

@movermeyer
Copy link
Author

@movermeyer movermeyer commented Jun 5, 2017

I now understand that it is a agnostic class and that it might be a non-trivial change.

But from a user friendliness perspective, this error message is definitely not ideal.

@movermeyer
Copy link
Author

@movermeyer movermeyer commented Jun 5, 2017

Other similar messages:

print(BitsInteger(8).parse('111'))

gives

Traceback (most recent call last):
  File "test.py", line 36, in <module>
    print BitsInteger(8).parse('111')
  File "env/lib/python2.7/site-packages/construct/core.py", line 165, in parse
    return self.parse_stream(BytesIO(data), context, **kw)
  File "env/lib/python2.7/site-packages/construct/core.py", line 176, in parse_stream
    return self._parse(stream, context, "parsing")
  File "env/lib/python2.7/site-packages/construct/core.py", line 576, in _parse
    data = _read_stream(stream, length)
  File "env/lib/python2.7/site-packages/construct/core.py", line 74, in _read_stream
    raise FieldError("could not read enough bytes, expected %d, found %d" % (length, len(data)))
construct.core.FieldError: could not read enough bytes, expected 8, found 3
print(BitStruct(BitsInteger(7)).parse(b'\xFF'))

gives

Traceback (most recent call last):
  File "test.py", line 32, in <module>
    BitStruct(BitsInteger(4)).parse(b'\xFF')
  File "env/lib/python2.7/site-packages/construct/core.py", line 165, in parse
    return self.parse_stream(BytesIO(data), context, **kw)
  File "env/lib/python2.7/site-packages/construct/core.py", line 176, in parse_stream
    return self._parse(stream, context, "parsing")
  File "env/lib/python2.7/site-packages/construct/core.py", line 1792, in _parse
    self.stream2.close()
  File "env/lib/python2.7/site-packages/construct/lib/bitstream.py", line 43, in close
    raise ValueError("closing stream but %d unread bytes remain, %d is decoded unit" % (len(self.rbuffer), self.decoderunit))
ValueError: closing stream but 4 unread bytes remain, 1 is decoded unit
@arekbulski
Copy link
Member

@arekbulski arekbulski commented Jun 5, 2017

There is no point adding more examples. I wont be able to change these messages. All these classes are bit/byte agnostic. They DO NOT KNOW the difference. I wont close the issue because that could be considered rude, but its a mute point.

@movermeyer
Copy link
Author

@movermeyer movermeyer commented Jun 6, 2017

It could be done without too many lines changed. I've made a commit to my fork that implements it.
I think it's not too bad, but as you're the maintainer, I will respect your decision here.

If you're interested, I'll make it a Pull Request.

@arekbulski
Copy link
Member

@arekbulski arekbulski commented Jun 6, 2017

Hmm I am in a good mood so yeah sure, send me a PR. At least I can take a look and thank you for the effort, if not merge it. 😄

@arekbulski
Copy link
Member

@arekbulski arekbulski commented Jun 6, 2017

Took a look at your link. Hah this fix was so simple I didnt even think of it. I like it, sure I can merge it. Only one small request, can you make string literals consistent? Double quotes are used throughout the codebase.

@arekbulski
Copy link
Member

@arekbulski arekbulski commented Jun 6, 2017

Very good PR, much welcomed. I personally see this bytes/bits issue mute, to me its kinds same-same but I see why other people might prefer the error messages to be more precise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.