-
Notifications
You must be signed in to change notification settings - Fork 31
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
Adopt a system of exceptions derived from KaitaiStructError #80
base: master
Are you sure you want to change the base?
Conversation
* Resolves #40 * Resolves #41 As explained in #40, this makes it easy to handle to all errors caused by invalid input data by using `kaitaistruct.KaitaiStructError` in a `try..except` statement. Three new exception types were added: `InvalidArgumentError`, `EndOfStreamError` and `NoTerminatorFoundError`. All changes to raised exceptions in this commit should be backward compatible, as we are only moving to subclasses of previously raised exceptions. `NoTerminatorFoundError` is a subclass of `EndOfStreamError` to address the suggestion in #41. Note that the `process_rotate_left` method could only raise `NotImplementedError` if someone called it manually (because KSC-generated parsers hardcode `group_size` to `1`, see https://github.com/kaitai-io/kaitai_struct_compiler/blob/c23ec2ca88d84042edba76f70c1f003d062b7585/shared/src/main/scala/io/kaitai/struct/languages/PythonCompiler.scala#L211), so it makes no sense to raise an exception derived `KaitaiStructError` (it's a programmer error, not a user input error). Most of our runtime libraries in other languages don't even have this `group_size` parameter, and if they do (C#, Java, Ruby), they also throw the equivalent of `NotImplementedError` (except the JavaScript runtime, which throws a plain string, which is _possible_ in JS but considered bad practice, so we should fix this).
@@ -128,6 +128,9 @@ def is_eof(self): | |||
return self._io.tell() >= self.size() | |||
|
|||
def seek(self, n): | |||
if n < 0: | |||
raise InvalidArgumentError("cannot seek to invalid position %d" % (n,)) |
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.
For the record, this exception was already thrown by the io.IOBase.seek()
method in the Python standard library. The exception type and message text and depended on which underlying stream was used. When using io.BytesIO
, a ValueError("negative seek value %zd")
was thrown (see Modules/_io/bytesio.c:659-660
). But when using a regular file, I was getting OSError: [Errno 22] Invalid argument
on both Windows and Linux.
So it seems better to do this check ourselves, if only for the sake of consistent behavior. And also because a negative seek position can easily be a result of invalid input data (and thus the raised exception should be a subclass of KaitaiStructError
) - imagine a situation like this:
meta:
id: negative_seek
seq:
# Consider parsing a byte with the highest bit set, i.e. `80..ff` in hex
- id: ofs_foo
type: s1
instances:
foo:
pos: ofs_foo
type: u1
If ofs_foo
is negative and we attempt to read foo
, the error that occurs is a clear parse error.
As explained in #40, this makes it easy to handle to all errors caused by invalid input data by using
kaitaistruct.KaitaiStructError
in atry..except
statement. Three new exception types were added:InvalidArgumentError
,EndOfStreamError
andNoTerminatorFoundError
. All changes to raised exceptions in this commit should be backward compatible, as we are only moving to subclasses of previously raised exceptions.NoTerminatorFoundError
is a subclass ofEndOfStreamError
to address the suggestion in #41.Note that the
process_rotate_left
method could only raiseNotImplementedError
if someone called it manually (because KSC-generated parsers hardcodegroup_size
to1
, see https://github.com/kaitai-io/kaitai_struct_compiler/blob/c23ec2ca88d84042edba76f70c1f003d062b7585/shared/src/main/scala/io/kaitai/struct/languages/PythonCompiler.scala#L211), so it makes no sense to raise an exception derivedKaitaiStructError
(it's a programmer error, not a user input error). Most of our runtime libraries in other languages don't even have thisgroup_size
parameter, and if they do (C#, Java, Ruby), they also throw the equivalent ofNotImplementedError
(except the JavaScript runtime, which throws a plain string, which is possible in JS but considered bad practice, so we should fix this).