Skip to content

Conversation

@irfan-dahir
Copy link
Contributor

This addresses #433

Important notes

  • Refactors limit usage a bit so the default 25 is not set if no limit property is in query
  • The max limit for Random endpoints is 5 to test the waters. There is a performance drop associated with a large sample size. Larger datasets like Users response slower compared to Anime.
  • Fixes a bug in MaxResultsPerPageRule and allows override from the MaxLimitWithFallback trait

Additional notes

  • It was discovered that applying the $defaultLimit override does not work in DTOs
    if (property_exists(static::class, "limit") && !$properties->has("limit")) {
    $properties->put("limit", max_results_per_page(
    property_exists(static::class, "defaultLimit") ? static::$defaultLimit : null));
}

max_results_per_page(property_exists(static::class, "defaultLimit") ? static::$defaultLimit : null))
max_results_per_page does 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\Optional within the Handler.

@irfan-dahir irfan-dahir requested a review from a team as a code owner November 9, 2024 20:58
@irfan-dahir irfan-dahir requested a review from pushrbx November 13, 2024 16:30
@irfan-dahir
Copy link
Contributor Author

Also resolves #490

@pushrbx
Copy link
Collaborator

pushrbx commented Nov 13, 2024

These changes seem to break the anime filtering and anime season logic. See tests for details. 😟

@irfan-dahir
Copy link
Contributor Author

irfan-dahir commented Nov 13, 2024

Woops, missed this. Looking into.

@codecov
Copy link

codecov bot commented Nov 13, 2024

Codecov Report

❌ Patch coverage is 50.90909% with 81 lines in your changes missing coverage. Please review.
✅ Project coverage is 56.81%. Comparing base (f9d6f87) to head (39f18c2).
⚠️ Report is 6 commits behind head on master.

Files with missing lines Patch % Lines
app/Http/Controllers/V4DB/RandomController.php 0.00% 10 Missing ⚠️
app/Features/QueryRandomAnimeListHandler.php 22.22% 7 Missing ⚠️
app/Features/QueryRandomMangaListHandler.php 22.22% 7 Missing ⚠️
app/Features/QueryRandomCharacterHandler.php 0.00% 6 Missing ⚠️
app/Features/QueryRandomPersonHandler.php 0.00% 6 Missing ⚠️
app/Features/QueryRandomCharacterListHandler.php 28.57% 5 Missing ⚠️
app/Features/QueryRandomMangaHandler.php 0.00% 5 Missing ⚠️
app/Features/QueryRandomPersonListHandler.php 28.57% 5 Missing ⚠️
app/Features/QueryRandomUserListHandler.php 28.57% 5 Missing ⚠️
app/Features/QueryRandomAnimeHandler.php 0.00% 4 Missing ⚠️
... and 6 more
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

{
$requestParams = collect($request->all());
$limit = $requestParams->get("limit");
$limit = $request->limit instanceof Optional ? max_results_per_page() : $request->limit;
Copy link
Collaborator

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?

@pushrbx
Copy link
Collaborator

pushrbx commented Nov 13, 2024

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 0.00% coverage from the Codecov report. (excl. RandomController)

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.

3 participants