-
Notifications
You must be signed in to change notification settings - Fork 8
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
Make error handling part of the public interface #25
Conversation
92e1377
to
8b8bc2d
Compare
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.
Good initiative!
@@ -38,6 +38,17 @@ def lazy_read(filelike, fileformat=None): | |||
:param fileformat: Either ecl_data_io.Format.FORMATTED for ascii | |||
format, ecl_data_io.Format.UNFORMATTED for binary formatted files | |||
or None for guess. | |||
|
|||
:raises ecl_data_io.EclParsingError: If the file is not a valid | |||
ecl file. |
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.
Is ecl
always referring to Eclipse? If so, why don't we write the full name?
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.
ecl is the name of the group that made eclipse. Its a convention now, perhaps something to change, but that is perhaps a bigger issue than this PR as ecl is in the name of the package.
built-in `open()` function may be raised. | ||
|
||
When given a stream, the exceptions associated with the stream will | ||
pass through. |
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.
Would it be possible to have a test for error handling? It would be easier to visualise what you mean by the condition file
opposed to stream
.
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.
I have added some doctests for this
built-in `open()` function may be raised. | ||
|
||
When given a stream, the exceptions associated with the stream will | ||
pass through. |
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.
Same as above.
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.
Also added doctest for this.
Introduces doctest to the rst files (which found some errors). Makes error handling more consistent for write operations. There is a breaking change where the custom error are now a subtype of value-error which could break some existing error handling.
756a34d
to
ee6950a
Compare
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.
Very nice PR!
No description provided.