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

feat: Added option to disable caching #145

Merged
merged 1 commit into from
Nov 5, 2024

Conversation

ricardoboss
Copy link
Contributor

This would fix #144

Copy link
Contributor

@Nexushunter Nexushunter left a comment

Choose a reason for hiding this comment

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

These changes look good. Can you include tests to ensure previous functionality isn't broken.

@gibahjoe
Copy link
Owner

gibahjoe commented Nov 4, 2024

If I may ask @ricardoboss , what is the use case of disabling caching?

@ricardoboss
Copy link
Contributor Author

My spec is a local file. That doesn't need caching.

@gibahjoe
Copy link
Owner

gibahjoe commented Nov 4, 2024

In this case, caching is used to store a copy of the API specification (spec) from the last time the generator was run. By comparing the current API spec with the cached version, the system can detect if any changes have been made. If the spec hasn’t changed, the generator skips the code regeneration step, which avoids unnecessary work.

This approach is especially useful when:

The API spec is large: Regenerating code for a large spec can be time-consuming.
Other code generation tools are in use: For instance, if you run build_runner to generate code for JsonSerializable, the API generator won’t unnecessarily regenerate the SDK, as it can see that the spec remains unchanged.

Maybe i will give it another name as caching doesnt quite describe its function.

@ricardoboss
Copy link
Contributor Author

That all sounds great in theory. For my use case however, it's not great.

My spec file changes frequently as I am developing the API. Sometimes the system doesn't detect it has changed. (at least as far as I can remember, I've been using my fork for a few months by now)

Look, adding this change has no impact for people who don't want it but gives people who want it a way to get it. It's also not hard to support this. Why the push back?

@gibahjoe
Copy link
Owner

gibahjoe commented Nov 4, 2024

You are right. just wanted to know the use case so I can find an appropriate name for the flag. Thanks for your contribution.

@gibahjoe
Copy link
Owner

gibahjoe commented Nov 4, 2024

Please resolve merge conflicts so this can go out in next release.

@ricardoboss ricardoboss force-pushed the issues/144-disable-cache branch from 4d5daaf to d1ae279 Compare November 4, 2024 22:53
@ricardoboss ricardoboss changed the title Added option to disable caching feat: Added option to disable caching Nov 4, 2024
Copy link

codecov bot commented Nov 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.06%. Comparing base (8f89ea3) to head (d1ae279).
Report is 20 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #145      +/-   ##
==========================================
+ Coverage   83.35%   84.06%   +0.70%     
==========================================
  Files          10       10              
  Lines         631      640       +9     
==========================================
+ Hits          526      538      +12     
+ Misses        105      102       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@gibahjoe gibahjoe merged commit 7f8770b into gibahjoe:master Nov 5, 2024
7 of 9 checks passed
@ricardoboss ricardoboss deleted the issues/144-disable-cache branch November 5, 2024 01:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add option to disable caching
3 participants