Add limit and max_age params to RSS feeds #7117
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
alexbecker
force-pushed
the
time-based-rss
branch
from
eec80b9
to
edac6c3
Compare
alexbecker
force-pushed
the
time-based-rss
branch
from
edac6c3
to
acfc23f
Compare
There was a problem hiding this comment.
Thanks for this @alexbecker 🌟
LGTM just left a minor comment and some nits. 👍
| # Return default if 'param' is absent or has an empty value. | ||
| return default | ||
| try: | ||
| return int(value) |
There was a problem hiding this comment.
Maybe it'd be worth asserting it must be a positive integer as well?
| MAX_RESULTS = 200 | ||
|
|
||
|
|
||
| def _get_int_query_param(request, param, default=None): |
There was a problem hiding this comment.
Nit: I feel defaulting to None might be dangerous here as the caller might expect an integer. The two options I see is raising an expection or returning a sentinel value (maybe -1 or 0 would make sense?).
|
I think we probably need a bit more discussion before continuing work on this, see #7116 (comment). |
miketheman
added
the
needs discussion
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Implements #7116, introducing
limitandmax_agequery parameters to thepackages.xmlandreleases.xmlfeeds. Thelimitparameter controls how many items are returned, defaulting to 40 if not present (same as current behavior) and respecting values up to 200 (an arbitrary maximum I chose, please suggest a more appropriate value). Themax_agequery parameter filters down to items at mostmax_ageseconds old; I chose to use seconds of age instead of datetimes/timestamps in the request because they are easier to parse (unless I used unix timestamps, I suppose) and less sensitive to clock skew.Creates a small
_get_int_query_paramutility to parse these parameters and 400 if they are not valid integers. Please let me know if there is an existing utility I should use or if I should move this to theutilsmodule.Currently
limitrequests overMAX_RESULTS(200) are silently reduced to returningMAX_RESULTSitems. Should this be changed to a 400 error?Thank you for maintaining PyPI and thanks in advance for your time reviewing this PR!