-
-
Notifications
You must be signed in to change notification settings - Fork 9
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
feat: random entry #113
feat: random entry #113
Conversation
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.
@dag7dev just an idea: maybe instead of ad-hoc endpoint random could simply be a query param that is available on the search endpoint? E.g.:
/search?random=true
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.
Perfect! Just a minor comment to go ;)
@@ -171,7 +174,7 @@ def search_entries(request): | |||
# total number of pages | |||
"page_total": paginator.num_pages, | |||
# current request page | |||
"page_current": page, | |||
"page_current": int(page), |
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.
Why are casting to int here? Pagination is already trying to the desired page here so if something is wrong (e.g. page is a string) it already fails there (and goes to page 1)
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.
it will return a string otherwise, and... this is not consistent with the normal backend behavior
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.
What? That's not true. At this line, page is used to paginate, if it's not a proper integer it would've failed there, so there's no way the code progress with page being a string.
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.
Please also make sure to run pre-commit, as the checks are failing due to some unformatted file
not sure why it hasn't been ran automatically |
I'm pretty sure about it. When I have some time, I will send a screenshot to clarify this
…-------- Messaggio originale --------
18/05/24 22:22, Antonio Vivace ha scritto:
@avivace commented on this pull request.
---------------------------------------------------------------
In [hhub/views.py](#113 (comment)):
> @@ -171,7 +174,7 @@ def search_entries(request):
# total number of pages
"page_total": paginator.num_pages,
# current request page
- "page_current": page,
+ "page_current": int(page),
What? That's not true. At [this line](https://github.com/gbdev/homebrewhub/blob/main/hhub/views.py#L54), page is used to paginate, if it's not a proper integer it would've failed there, so there's no way the code progress with page being a string.
—
Reply to this email directly, [view it on GitHub]
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
@avivace take a look. This is without the "int" |
Ah alright, I understood what you mean, it's on the way back on the response |
Description
Closes Play random entry #71