add base classes from rio-tiler-pds #273
Conversation
| def __exit__(self, exc_type, exc_value, traceback): | ||
| """Support using with Context Managers.""" | ||
| pass | ||
|
|
There was a problem hiding this comment.
@geospatial-jeff I know we agreed to removes those (done in #271) But after some though I think it's not a great Idea.
The main reason to remove those was to ease the use for Async but I do feel using BaseReader is not the way forward for this and we should create a AsyncBaseReader).
The major problem by not having ctx has a requirement is that a user could create a Reader without them but then it won't be able to use it in most of our other project that use with reader() as ...
There was a problem hiding this comment.
There is a separate AsyncBaseReader here. Thinking through this again I agree that these should be here. Its standard to have post init logic in __init__ and keep context managers simple. But because there is no way to execute asynchronous code within __init__ we have to run post init logic in __aenter__ and __aexit__ for the async base class. Note these methods aren't present on the implementation linked above but they should be.
So if we are requiring the async base to be a context manager the sync base should also be to keep the interfaces consistent. I guess there is one difference; the async base won't be usable outside the context manager, but we could provide a class method which people could call that wraps any post-init logic.
reader = await AsyncBaseReader.open()
await reader.close()There was a problem hiding this comment.
🙏 thanks @geospatial-jeff
This PR moves base classes from rio-tiler-pds to rio-tiler
ref: cogeotiff/rio-tiler-pds#24