Add non skip take pagination #341
Conversation
|
Also i just saw the ruby support is for the project is |
|
Looks good 👍 The From the other side I like the idea to not modify them and just add new methods ( |
lib/dynamoid/criteria/chain.rb
Outdated
| @@ -140,13 +140,13 @@ def records | |||
| records_via_query | |||
| else | |||
| records_via_scan | |||
| end | |||
| end.lazy.flat_map(&:itself) | |||
There was a problem hiding this comment.
records_via_query /records_via_scan mean "records" not "pages".
So it's reasonable either to make them return lazy collection of hashed (like they did before) or rename them to pages_via_query/pages_via_scan
There was a problem hiding this comment.
Sounds good, this afternoon is gonna be moved to fixing stuff like that and creating the final paged public method. Yesterday was just getting my hooks into it all and specs green again.
Just didn't want to get too far before showing this since it is a fairly fundamental change in internal datagram
| Address.find_by_pages do |addresses| | ||
| # have an array of pages | ||
| end | ||
| ``` |
There was a problem hiding this comment.
My next PR adding the restart hashes will continue with this section for both fixed limit and native pages.
| @@ -128,6 +128,10 @@ def each(&block) | |||
| records.each(&block) | |||
| end | |||
|
|
|||
| def find_by_pages(&block) | |||
There was a problem hiding this comment.
What is the final verdict on the page for this, if you would prefer pages I can move that method up
There was a problem hiding this comment.
The name find_by_pages looks good.
| @@ -136,27 +140,41 @@ def each(&block) | |||
| # | |||
| # @since 0.2.0 | |||
| def records | |||
| pages.lazy.flat_map { |i| i } | |||
There was a problem hiding this comment.
the explicit lazy turns out to be needed here, or the flat_map ended up calling scan/query
|
@andrykonchin Taking this off Draft status and going to branch to work on |
|
Eh this needs more work. something I did make jruby not like the enums wraped in enums. of course fixing that in jruby angered mri. |
This reverts commit 0aaddf5.
|
So there is a difference between MRI and JRuby with yeilding to an enumerator and this is why my change of yielding an array is failing. See these examples below. # jruby
Enumerator.new do |y|
y << [ 1234 ]
end.to_a
# => [[[1234]]]# mri
Enumerator.new do |y|
y << [ 1234 ]
end.to_a
# => [[1234]]Do you have any idea what is up with this? I'm going to try converting to enum_for and normal yield to see if that fixes it. Looks like it is a bug with So turns out the bug is with single element arrays, and it is causing more widespread issues. I also found this issue in the JRuby tracker. I'll give enum_for a try, but this may be a show stopper for jruby support in this PR. Enumerator.new {|y| y.yield [1]}.first #=> 1, it should be [1]Thoughts? |
Actually support of the old versions was ended in v3.0. So According to Readme.md:
|
It's definitely an actual bug in JRuby and looks like Good job 👍 |
|
So I turns out the bug is not present using In order to have the scan warning issued on creation but before hydration I had to move it around. If you are okay with it only showing once iteration begins, it can be moved after the end _for and the spec updated Edit: wrote this on a stale page, didn't see the merge |
Partial completion of #338
This is still in progress, but should have 100% of tests passing. I wanted to put it up to show the approach I have taken incase I'm headed down a path you disagree with.
Basics:
Scan and query now return page at at time. I didn't find documentation on scan and query, so I assume it is an internal method. However if you have been advising users to leverage scan and query directly, this would be a breaking change for them. In this case I can rename them to scan_page etc and maintain existing query/scan behaviors.
Arrays are bubbled up and flattened at the last minute outbound.
At this point adding an each_page, or find_in_pages method is fairly trivial (which will also remove the method calls trailing the
end).Most of the changes in this PR are making the specs green with as little change as possible.
I will submit the restart hash generation method as a separate PR.