Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Increase MAX_LIMIT constant to 100 and DEFAULT_LIMIT to 30 #80

Merged
merged 1 commit into from
Feb 7, 2022
Merged

Increase MAX_LIMIT constant to 100 and DEFAULT_LIMIT to 30 #80

merged 1 commit into from
Feb 7, 2022

Conversation

ueco-jb
Copy link

@ueco-jb ueco-jb commented Feb 4, 2022

closes #76

@ueco-jb ueco-jb self-assigned this Feb 4, 2022
uint
uint approved these changes Feb 7, 2022
View reviewed changes
Copy link
Contributor

@uint uint left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Makes me realize we never test the max limit though, since this PR breaks no tests. Huh.

@ueco-jb
Copy link
Author

ueco-jb commented Feb 7, 2022

We test pagination with our own limits, so unless such line

limit.unwrap_or(DEFAULT_LIMIT).min(MAX_LIMIT)

can contain bug, I guess we're safe. :) Although sure, for mind clarity and coverage sake we could add more testcases.

I'm just waiting for @ethanfrey to take a look, because I took liberty to increase DEFAULT_LIMIT as well (it wasn't part of issue).

ethanfrey
ethanfrey approved these changes Feb 7, 2022
View reviewed changes
Copy link
Contributor

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm. seem like reasonable numbers.

I don't feel that test is essential, but in some contracts I think

limit.unwrap_or(DEFAULT_LIMIT).min(MAX_LIMIT) as usize; was pulled out into it's own functions which could easily be tested in simple unit tests.

@ueco-jb ueco-jb merged commit 6d006c3 into main Feb 7, 2022
@ueco-jb ueco-jb deleted the 76-increase-max-limit-to-100 branch February 7, 2022 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@uint uint uint approved these changes

@ethanfrey ethanfrey ethanfrey approved these changes

@maurolacy maurolacy maurolacy approved these changes

Assignees

@ueco-jb ueco-jb

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Increase max limit
4 participants
@ueco-jb @uint @ethanfrey @maurolacy