Support parsing batch results #146

gabisurita · 2017-05-21T20:20:25Z

Fixes/Updates #145

This may not be the best API for this feature, but it does support the use case for dumping records on kinto-wizard (Kinto/kinto-wizard#16).

A small example:

from kinto_http import Client

client = Client('http://localhost:8888/v1',
                auth=('gabi', 'ilovecats'),
                collection='status')

with client.batch() as batch:
    batch.update_record(id='r1', data={'busy': True})
    batch.get_record('r1')

set_r1, get_r1 = batch.results()
assert set_r1 == get_r1

Todos:

Add unit tests
Update CHANGELOG and README

Opinions?

Support parsing results from batch requests with a parse_results method and a results attribute on a Batch context client after the batch operation is sent.

leplatrem

Mostly wording :)

Thanks !!!

leplatrem · 2017-05-22T09:30:02Z

CHANGELOG.rst

@@ -7,7 +7,7 @@ This document describes changes between each past release.
 8.1.0 (unreleased)
 ==================

- Nothing changed yet.
+- Allow reading from batch contexts using the ``parse_results`` method. (#146)


I would suggest to be more human friendly with something like:

Allow reading responses from batch requests with the ``results()`` method. (#146)

leplatrem · 2017-05-22T09:30:36Z

README.rst

          batch.update_record(data={'id': idx})

-A batch object shares the same methods as another client.
+Reading data from batch operations is achived by using the ``parse_results`` method


when mentionning a method, it's nicer if we put the parenthesis ()

leplatrem · 2017-05-22T09:32:08Z

kinto_http/batch.py

+            self.results.append((resp, headers))
+        return self.results
+
+    def parse_results(self):


results() should be enough IMO. Parsing is not truely the action here

leplatrem · 2017-05-22T09:33:01Z

kinto_http/batch.py

+
+    def parse_results(self):
+        # Get each batch block response
+        block_responses = [resp for resp, _ in self.results]


chunks would be easier to understand since we use this term previously

leplatrem · 2017-05-22T09:36:19Z

kinto_http/tests/test_batch.py

@@ -112,3 +112,25 @@ def test_batch_raises_exception_as_soon_as_subrequest_fails(self):
        with self.assertRaises(KintoException):
            batch.send()
        assert self.client.session.request.call_count == 1
+
+    def test_results_attribute_is_available(self):


I'm not sure if we have both. Especially because this results attributes is chunked. I know we'd lose the headers information, but I would rather make it private. Especially if we don't make it official in docs.

glasserc · 2017-05-22T23:56:06Z

I am not crazy about the API because it seems forwards-incompatible for doing things like sending a batch of updates as part of an offline-first sync() call. I get that simple things should be simple, but this makes complicated things a little harder. But we can always just break compatibility later..

gabisurita · 2017-05-23T02:48:56Z

I am not crazy about the API because it seems forwards-incompatible for doing things like sending a batch of updates as part of an offline-first sync() call. I get that simple things should be simple, but this makes complicated things a little harder. But we can always just break compatibility later..

I agree this API is limited, but honestly I think we should support both a simple way to fetch only the response body and another to fetch the full response (mostly for internal stuff). I can't see a way to have an easy to use and fully forward-compatible call, do you have something in mind?

leplatrem

A few nits left, but then I'm ok to merge :)

leplatrem · 2017-05-23T07:40:44Z

README.rst

+      batch.get_record('r2')
+      batch.get_record('r3')
+
+  r1, r2, r3 = batch.parse_results()


leftover :)

leplatrem · 2017-05-23T07:40:51Z

README.rst

          batch.update_record(data={'id': idx})

-A batch object shares the same methods as another client.
+Reading data from batch operations is achived by using the ``results()`` method
+available ater a batch context is closed.


achieved

after

leplatrem · 2017-05-23T07:40:57Z

README.rst

+
+  r1, r2, r3 = batch.parse_results()
+
+Besides the ``parse_results`` method, a batch object shares all the same methods as


leplatrem · 2017-05-23T07:41:51Z

kinto_http/__init__.py

@@ -97,6 +97,10 @@ def batch(self, **kwargs):
        batch_session = BatchSession(self,
                                     batch_max_requests=batch_max_requests)
        batch_client = self.clone(session=batch_session, **kwargs)
+
+        # set a reference for reading results from the context


µnit: capitalize first word and add trailing .

leplatrem · 2017-05-23T07:43:13Z

kinto_http/batch.py

@@ -43,7 +44,6 @@ def _build_requests(self):
        return requests

    def send(self):
-        result = []


don't we want to reset self._results here?

Side note: dont code in a hurry...

glasserc · 2017-05-23T15:51:21Z

I can't see a way to have an easy to use and fully forward-compatible call, do you have something in mind?

What I would normally do is return a Response object which has a body attribute and/or a json property/method. But this is fine for now. Nice to see a PR from you again :)

gabisurita added 3 commits May 21, 2017 16:55

upporting parsing results from batch requests

afaf70f

Support parsing results from batch requests with a parse_results method and a results attribute on a Batch context client after the batch operation is sent.

Merge branch 'master' of https://github.com/Kinto/kinto-http.py

b32047e

add a functional test

92ca18d

gabisurita requested a review from leplatrem May 21, 2017 20:21

gabisurita added 2 commits May 21, 2017 22:09

add unit tests

fe227e9

Update docs

704fa97

gabisurita changed the title ~~[WIP] Support parsing batch results~~ Support parsing batch results May 22, 2017

leplatrem requested changes May 22, 2017

View reviewed changes

gabisurita force-pushed the feat-parse-batch-results branch 2 times, most recently from 56584b1 to f46a1e6 Compare May 23, 2017 02:25

@leplatrem comments

ae732c5

gabisurita force-pushed the feat-parse-batch-results branch from f46a1e6 to ae732c5 Compare May 23, 2017 02:38

leplatrem approved these changes May 23, 2017

View reviewed changes

More comments

ed59f06

Side note: dont code in a hurry...

leplatrem approved these changes May 23, 2017

View reviewed changes

leplatrem merged commit 8fc639e into master May 23, 2017

leplatrem deleted the feat-parse-batch-results branch May 23, 2017 13:08

leplatrem mentioned this pull request May 25, 2017

Add support for batch read operations #145

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support parsing batch results #146

Support parsing batch results #146

gabisurita commented May 21, 2017 •

edited

Loading

leplatrem left a comment

leplatrem May 22, 2017

leplatrem May 22, 2017

leplatrem May 22, 2017 •

edited

Loading

leplatrem May 22, 2017

leplatrem May 22, 2017

glasserc commented May 22, 2017

gabisurita commented May 23, 2017 •

edited

Loading

leplatrem left a comment

leplatrem May 23, 2017

leplatrem May 23, 2017

leplatrem May 23, 2017

leplatrem May 23, 2017

leplatrem May 23, 2017

glasserc commented May 23, 2017


		r1, r2, r3 = batch.parse_results()

		Besides the ``parse_results`` method, a batch object shares all the same methods as

Support parsing batch results #146

Support parsing batch results #146

Conversation

gabisurita commented May 21, 2017 • edited Loading

leplatrem left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leplatrem May 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glasserc commented May 22, 2017

gabisurita commented May 23, 2017 • edited Loading

leplatrem left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glasserc commented May 23, 2017

gabisurita commented May 21, 2017 •

edited

Loading

leplatrem May 22, 2017 •

edited

Loading

gabisurita commented May 23, 2017 •

edited

Loading