-
-
Notifications
You must be signed in to change notification settings - Fork 270
✨Feature: Allows random endpoints to return multiple items #559
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
base: master
Are you sure you want to change the base?
Conversation
|
Also resolves #490 |
|
These changes seem to break the anime filtering and anime season logic. See tests for details. 😟 |
|
Woops, missed this. Looking into. |
…jikan-rest into feature/multiplerandom
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #559 +/- ##
============================================
- Coverage 57.23% 56.81% -0.42%
- Complexity 1391 1420 +29
============================================
Files 340 344 +4
Lines 5670 5741 +71
============================================
+ Hits 3245 3262 +17
- Misses 2425 2479 +54 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| { | ||
| $requestParams = collect($request->all()); | ||
| $limit = $requestParams->get("limit"); | ||
| $limit = $request->limit instanceof Optional ? max_results_per_page() : $request->limit; |
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.
This is repeating code, shouldn't we either change the PreparesData to replace Optional with this?
|
Can you also add tests for the new endpoints please? 😄 I don't want to break them in the future 😄 Just add tests to the files with |
This addresses #433
Important notes
limitusage a bit so the default25is not set if nolimitproperty is in querylimitfor Random endpoints is 5 to test the waters. There is a performance drop associated with a large sample size. Larger datasets likeUsersresponse slower compared toAnime.MaxResultsPerPageRuleand allows override from theMaxLimitWithFallbacktraitAdditional notes
$defaultLimitoverride does not work in DTOsmax_results_per_page(property_exists(static::class, "defaultLimit") ? static::$defaultLimit : null))max_results_per_pagedoes not override with $defaultLimit`. The return value will always be what's in the config. We'll need to approach this differently in case we need to allow for dynamic overrides per DTO command.I've commented out this part. We should probably check for
$limit instanceof Spatie\LaravelData\Optionalwithin the Handler.