CLN: Add types gridprop-import-xtgcpprop #1055
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1055 +/- ##
==========================================
- Coverage 80.05% 79.97% -0.08%
==========================================
Files 94 94
Lines 13534 13555 +21
Branches 2240 2245 +5
==========================================
+ Hits 10835 10841 +6
- Misses 1960 1973 +13
- Partials 739 741 +2 ☔ View full report in Codecov by Sentry. |
| @@ -53,33 +71,39 @@ def import_xtgcpprop(mfile, ijrange=None, zerobased=False): | |||
| # read metadata which will be at position offet + nfloat*narr +13 | |||
| pos = offset + nbyte * narr + 13 | |||
|
|
|||
| with open(mfile.file, "rb") as fhandle: | |||
| fhandle.seek(pos) | |||
| jmeta = fhandle.read().decode() | |||
There was a problem hiding this comment.
no need to decode, json.loads can deal with bytes and strings.
|
|
||
| # unpack header | ||
| swap, magic, nbyte, ncol, nrow, nlay = unpack("= i i i q q q", buf) | ||
| if not isinstance(mfile.file, Path): |
There was a problem hiding this comment.
Im not sure what todo in this case, i wonder if we should add a file-reader/opener context manger to the _XTGeoFile class that provides a filelike interface when reading.
Another option is that the types on _XTGeoFile.file is wrong, and it can only be Path?
There was a problem hiding this comment.
It is valid for it to be a memory stream. I would suggest something like
if isinstance(mfile.file, Path):
with open(mfile.file, "rb") as f:
header = f.read(offset)
else:
header = mfile.file.read(offset) # might need to handle for StringIO
mfile.file.seek(0) # rewind
swap, magic, nbyte, ncol, nrow, nlay = unpack("= i i i q q q", header)There was a problem hiding this comment.
I do think a context manager for header reads would be valuable generally, though
janbjorge
requested review from
jcrivenaes,
eivindjahren and
mferrera
and removed request for
eivindjahren
| elif isinstance(filelike, BytesIO): | ||
| was_at = filelike.tell() | ||
| try: | ||
| yield filelike.read(offset) | ||
| finally: | ||
| filelike.seek(was_at) | ||
| elif isinstance(filelike, StringIO): | ||
| was_at = filelike.tell() | ||
| try: | ||
| yield filelike.read(offset).encode() | ||
| finally: | ||
| filelike.seek(was_at) |
There was a problem hiding this comment.
Could be combined into one-ish, but i like this more. I tried and it just looks like a mess.
| @@ -216,7 +215,7 @@ def __init__( | |||
| if isinstance(filelike, str): | |||
| filelike = pathlib.Path(filelike) | |||
|
|
|||
| self._file = filelike | |||
| self._file: pathlib.Path | io.BytesIO | io.StringIO = filelike | |||
There was a problem hiding this comment.
is there a reason why we don't use the FileLike defined in common.types here ?
There was a problem hiding this comment.
It includes str, in this case some code futher up the chain converts str to Path.
Addresses #1011