adrienverge / localstripe

A fake but stateful Stripe server that you can run locally, for testing purposes.
GNU General Public License v3.0
192 stars 59 forks source link

List pagination: starting_after #169

Closed tarasyarema closed 3 years ago

tarasyarema commented 3 years ago

implements starting_after query param for List as noted in #168

tarasyarema commented 3 years ago

Thanks for your comments! Good point with the call of has_more before data. I implemented a helper method _compute_starting_pos that computes the staring position before has_more and data. Plus, the way it fetches the list is with the getattr function as you mentioned. I put it this way because in the code there are some time that arbitrary elements are appended to the list after the init, so we need to check for the starting position every time (if given).

tarasyarema commented 3 years ago

Made a simple refactor. The value self._starting_pos is not really used as its dynamically computed, so it's transferred to the method self._starting_pos().

adrienverge commented 3 years ago

Thanks, this new version looks good!

Made a simple refactor.

I think it was better before this commit refactor: simplification, because when exported to JSON, computing the start index (which can be a heavy operation) is done twice, at least. With the previous version, it was done only once. Beware that the function name _compute_staring_pos() has a typo (missing t).

Can you also squash all commits into one with a consistent commit message?

tarasyarema commented 3 years ago

Done @adrienverge !

tarasyarema commented 3 years ago

Btw, don't know why the tests failed only for Python 3.8.

tarasyarema commented 3 years ago

I don't really get what you mean with the computing start... The function is invoked two times, but once the value is set we do not go in the loop, hence the heavy computation is only done once (?)

adrienverge commented 3 years ago

@tarasyarema you can add a debugging line to see whether the loop is entered only once, or more:

@@ -1556,6 +1556,7 @@ class List(StripeObject):
             return

         # Only traverse the list when starting_after is given
+        print('entering heavy computation...', flush=True)
         for i, item in enumerate(self._list):
             if getattr(item, 'id', None) == self._starting_after:
                 self._starting_pos = i + 1
tarasyarema commented 3 years ago

Oh sorry, I forgot about the _starting_after... Lemme change it

tarasyarema commented 3 years ago

Tested the different resources with starting_after parameter and it works only computing one time the _starting_pos.

adrienverge commented 3 years ago

@tarasyarema I asked you to double checked before I have to review yet another time, and there are still multiple problems:

tarasyarema commented 3 years ago

Pushed the changes. Few comments:

  1. I literally put a print like you mentioned in this comment and saw only one comment per request, but anyways... Implemented the way you mentioned it.
  2. Sorry about the comment, forgot to remove them.
tarasyarema commented 3 years ago

Thanks for merging! About the docker image, will it be updated too?

adrienverge commented 3 years ago

It will on the next release; probably after #165 and #167 and reviewed and merged.