-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement "default_max_page_size" #752
Conversation
@esjee would it be valuable to add it to the security section of the docs? And the change log? |
I think that makes sense! Would it also be helpful to have a sane default value here instead of |
@esjee that will be a breaking change plus I'm not sure what a sane value would be. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 great implementation and thanks for the solid tests!
I just have a couple of questions about those renamed variables.
@@ -187,14 +187,74 @@ def get_page_info(result) | |||
assert_equal(["Yavin", "Echo Base"], get_names(result)) | |||
assert_equal(false, get_page_info(result)["hasPreviousPage"], "hasPreviousPage is false when last is not specified") | |||
|
|||
third_cursor = "Mw==" | |||
fourth_cursor = "Mw==" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the table you posted in the description, "Mw=="
is the third row. Are you sure we should rename this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope! This is a rename gone badly. I was a bit too eager with the find-and-replace. Sorry!
@@ -206,23 +206,82 @@ def get_last_cursor(result) | |||
|
|||
it "applies to queries by `last`" do | |||
second_to_last_two_names = ["Death Star", "Shield Generator"] | |||
first_and_second_names = ["Yavin", "Echo Base"] | |||
first_second_and_third_names = ["Yavin", "Echo Base"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are only two items in this array, why should it be renamed to have and_third
in the variable name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above, this is a find-and-replace error. Sorry!
Makes sense, thanks for following up! |
#751
Usage:
For any relation where
max_page_size
is not defined, or if it isnil
, thedefault_max_page_size
will be used instead.Also, partially for my own benefit, here's the complete list of bases in order:
(Yavin being the first base, Headquarters being the last).