Refactor, better mode handling, deprecate asdf.AsdfFile.open #579
Conversation
|
Hi there @drdavella 👋 - thanks for the pull request! I'm just a friendly 🤖 that checks for issues related to the changelog. I help make sure that this pull request is milestoned and labeled correctly. This is mainly intended for the maintainers, so if you are not a maintainer you can ignore this, and a maintainer will let you know if any action is required on your part 😃. Everything looks good from my point of view! 👍 If there are any issues with this message, please report them here. |
asdf/asdf.py
Outdated
| if asdf_mode is None: | ||
| if isinstance(fileobj, str): | ||
| parsed = generic_io.urlparse.urlparse(fileobj) | ||
| if parsed.scheme == 'http': |
There was a problem hiding this comment.
What if the scheme is https
asdf/asdf.py
Outdated
| if not _compat: | ||
| mode = _check_and_set_mode(fd, mode) | ||
| if mode == 'r' and not copy_arrays: | ||
| copy_arrays = True |
There was a problem hiding this comment.
Should we print a warning here?
There was a problem hiding this comment.
Yes, but I just pushed a new set of changes that makes this irrelevant. I think the update contains a better solution to the issue you reported.
| @@ -1287,6 +1222,132 @@ def get_history_entries(self): | |||
| AsdfFile.keys.__doc__ = dict.keys.__doc__ | |||
|
|
|||
|
|
|||
| def _check_and_set_mode(fileobj, asdf_mode): | |||
|
|
|||
| if asdf_mode is not None and asdf_mode not in ['r', 'rw']: | |||
There was a problem hiding this comment.
Can we have r+ or w+ modes?
There was a problem hiding this comment.
Do you just mean that we should add these and make them equivalent to the current 'rw'? Although I think to be consistent with Python's open API, 'w+' would have to overwrite an existing file. Maybe we should support that, though?
There was a problem hiding this comment.
I wasn't sure if those modes were supported but not mentioned here or not supported at all. Looks like the latter case so all is good 👍
drdavella
force-pushed
the
refactor-open-handling
branch
from
a1ba05c to
92f0f88
Compare
asdf/tags/core/ndarray.py
Outdated
| self._make_array().__setitem__(*args) | ||
| except Exception: | ||
| self._array = None | ||
| raise |
There was a problem hiding this comment.
Since we've cut with 2.7:
except Exception as e:
self._array = None
raise e from Noneseems better - it does not pollute the error trace.
drdavella
force-pushed
the
refactor-open-handling
branch
3 times, most recently
from
195a029 to
8bb7399
Compare
This allows us to improve the way that file modes are handled.
Now the top-level function asdf.open should be used instead
drdavella
force-pushed
the
refactor-open-handling
branch
from
817e8c0 to
e17a376
Compare
This resolves #578 (albeit indirectly) and resolves #574.
It refactors the top-level
asdf.openfunction into a standalone factory function in order to enable better decisions about handling file modes, particularly with respect to memory mapping. It also deprecates the use ofasdf.AsdfFile.open, since this was not capable of handling such decisions.There's still a bit more work:
Add a warning when explicitly giving(no longer applies, see below)mode='r'withcopy_arrays=FalseEven though this is technically a bug fix, it affects the API and is a big enough change that it should go in 2.2.0.