Add some utility decorators for Model/States methods #247
Conversation
| @@ -137,6 +138,7 @@ cdef class States: | |||
| self._states.swap(states) | |||
| return move(states) | |||
|
|
|||
| @_file_object_arg("rb") # translate str/bytes file inputs into file objects | |||
There was a problem hiding this comment.
These are redundant for now, but will be used in #241
arcondello
force-pushed
the
feature/model-utility-decorators
branch
from
12bbd60 to
a8630a1
Compare
dwave/optimization/utilities.py
Outdated
| @functools.wraps(method) | ||
| def _method(cls_or_self, file, *args, **kwargs): | ||
| if isinstance(file, str): | ||
| with open(file, mode) as fobj: |
There was a problem hiding this comment.
open accepts not only str for file, but also bytes and a os.PathLike(i.e. pathlib.Path instance). Neither of these two are a str instance.
There was a problem hiding this comment.
Good call, os.PathLike especially should be covered. Will fix and add tests.
dwave/optimization/utilities.py
Outdated
| def decorator(method): | ||
| @functools.wraps(method) | ||
| def _method(cls_or_self, file, *args, **kwargs): | ||
| if isinstance(file, str): |
There was a problem hiding this comment.
I would consider inverting the test here to test for required file properties (like readable, writeable, binary), instead just str instance, as that would be a bit more flexible (to accept not only bytes, Paths, but also generators, etc).
Also, you could do _file_object_arg(read: bool = False, write: bool: False, binary: bool = True) for explicitness.
But I also appreciate the appeal of simplicity of the implemented approach.
There was a problem hiding this comment.
Yes, especially if we're also doing os.PathLike etc inverting makes sense.
Re the explicit input arguments, because this is private I am inclined to go with just mode. If we eventually make it public then we can consider more explicit arguments.
dwave/optimization/utilities.py
Outdated
| __all__ = [] | ||
|
|
||
|
|
||
| def _file_object_arg(mode): |
There was a problem hiding this comment.
Add type annotations here and below?
Previously they incorrectly claimed that we could accept ByteStrings.
arcondello
force-pushed
the
feature/model-utility-decorators
branch
from
4a48583 to
f800e22
Compare
These are used in #241, but are not really serialization-specific. So to make that PR easier to review IMO it makes sense to break them out.
The existing testing covers it pretty well, hence no new tests.