Feat/Context Managers for Document and Security Document

[adeelsohailahmed]

Adds asynchronous context managers to both Document and Security Document.

These context managers automatically:

• Retrieve the document from the remote server and overwrite local data, if document already exists
• Upon exit, update the remote document with the updated local data, creating a new document if it didn't exist before.
• Don't create/update the document if there were uncaught exceptions in the with block.

I've also added the appropriate tests.

An example:

async with CouchDB(server=SERVER_URL, user=USER, password=PASSWORD) as couchdb:
    db = await couchdb.create("test_db", exists_ok=True)
    async with Document(database=db, id="new_doc") as doc:
        doc["king"] = "elvis"

Inspired from (now deprecated) python-cloudant. Their examples for reference: Document Context Manager & Security Document Context Manager

PS: Thank you for this amazing library.

[bmario]

Thank you for your PR. Now that you brought it up, the missing context managers look like a major oversight on my side.

However, I'm having two concerns regarding your implementation. For aiocouch, I see using the constructors of the different classes more as an advanced feature. I want users to use the member functions. For example, use Database.__getitem__ instead of Document.__init__. That is certainly a different approach than python-cloudant. But context managers make so much sense, so I'd like to see them as return values for member functions as well.

The other thing I'm not convinced of is silently discarding a ConflictError in the __aexit__. That is surprising to users and may lead to data loss. Also, there is not even a way to detect the conflict error in those cases.

[codecov[bot]]

Codecov Report

Merging #35 (ea3af0b) into master (e6afd5d) will increase coverage by 0.03%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #35      +/-   ##
==========================================
+ Coverage   96.67%   96.70%   +0.03%     
==========================================
  Files          11       11              
  Lines         841      851      +10     
==========================================
+ Hits          813      823      +10     
  Misses         28       28              

Impacted Files	Coverage Δ
aiocouch/document.py	92.52% <100.00%> (+0.45%)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update e6afd5d...ea3af0b. Read the comment docs.

[adeelsohailahmed]

Thank you for your kind review!

May I suggest keeping the Database member functions as they are for more flexibility / finer control?

Can we think of Document / Security Document context managers as sort of an advanced feature? Due to the assumptions these context managers currently make, it's a tradeoff between convenience and finer control.

This is the sort of compromise that python-cloudant seems to have made as well.

(I'm not suggesting to follow python-cloudant for everything. I just love that, as a user, both aiocouch and python-cloudant are almost similar despite being very different under-the-hood).

As for ConflictError, my bad. Would this approach be alright (as shown in the example of aiocouch documentation)? Or should we simply let the ConflictError bubble up?

--- a/aiocouch/document.py
+++ b/aiocouch/document.py
@@ -97,7 +97,11 @@ class Document(RemoteDocument):
         traceback: Optional[TracebackType],
     ) -> None:
         if exc_type is None:
-            with suppress(ConflictError):
+            try:
+                await self.save()
+            except ConflictError:
+                info = await self.info()
+                self.rev = info["rev"]
                 await self.save()

[adeelsohailahmed]

@bmario Can you please spare some time to review the changes I've made?

[adeelsohailahmed]

I'm sorry, but I am at a loss here. I don't know why the Actions keep failing with Error: Resource not accessible by integration.

Mypy itself is not reporting any error:

{"name":"Mypy","head_sha":"e3c566051b46446098faf73f3ae72d778cb5f0fa","completed_at":"2021-08-05T12:30:00.907Z","conclusion":"success","output":{"title":"Mypy","summary":"There are 0 mypy warnings","annotations":[]}}

Am I missing something? Perhaps a token in my forked repo?

[bmario]

I'm sorry, but I am at a loss here. I don't know why the Actions keep failing with Error: Resource not accessible by integration.

Mypy itself is not reporting any error:

{"name":"Mypy","head_sha":"e3c566051b46446098faf73f3ae72d778cb5f0fa","completed_at":"2021-08-05T12:30:00.907Z","conclusion":"success","output":{"title":"Mypy","summary":"There are 0 mypy warnings","annotations":[]}}

Am I missing something? Perhaps a token in my forked repo?

Don't mind that. I think this is a GitHub Actions configuration issue on my part.

[adeelsohailahmed]

Thank you so much for taking the time to review the code! I've made changes as per your review.

[adeelsohailahmed]

I'd like to see the fixtures getting used in the one test case though

I'm using the database fixture in every test case I've created (unless you were referring to using filled_database fixture).

Also, can you add a paragraph to the documentation about context managers? That would be appreciated.

I've added a section about context managers to the documentation. The section also contains appropriate, self-contained examples. Please take a look, and do let me know if there are some changes I ought to make.

I've also edited the session.rst. Its example was missing async.

[bmario]

Looks good to me now. Thank you.