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.

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

Allow Custom Limiting in API #51

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

timroejr
Copy link

I was speaking with SizzlingCalamari about some things and this was one thing I suggested that he encouraged me to work with and do a pull request. I went ahead and allowed for a modifiable limiting factor when someone calls /api/matches. The user can choose /api/matches/3 if they only wanted the JSON to return 3 matches instead of the default 12. However if no matches are defined it should return with the default 12 matches. If something is wrong feel free to let me know.

var limitCount = req.params.limit;
if (limitCount < 1 || limitCount == null) {
limitCount = 12; //Default Value if one is not specified
}
Copy link
Member

Choose a reason for hiding this comment

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

We need a maximum on this value.
How about adding this line here:
limitCount = Math.min(limitCount, 100);

@dy-dx
Copy link
Member

dy-dx commented Dec 29, 2015

Hey thanks for the pull request! This is good to merge after the maximum thing is addressed.

if (limitCount < 1 || limitCount == null) {
limitCount = 12; //Default Value if one is not specified
}
limitCount = Math.min(limitCount, 100); //Limits Count to a Maximum of 100
Copy link
Author

Choose a reason for hiding this comment

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

Added the Maximum Value here to allow for a limit of a maximum of 100 matches.

@timroejr
Copy link
Author

Hey @dy-dx I went ahead and did as you suggested. Please let me know if you see anything else you would like to see changed! Thanks for taking the time to look at the pull request!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants