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: Use memoized request_class and response_class values #3205

Merged
merged 2 commits into from
Mar 16, 2024

Conversation

kedod
Copy link
Contributor

@kedod kedod commented Mar 15, 2024

Description

  • Use memoized request_class and response_class values

Closes

@kedod kedod requested review from a team as code owners March 15, 2024 00:19
@kedod kedod force-pushed the use-memoized-request-class branch from 94e6c71 to ee9d4ca Compare March 15, 2024 00:20
@kedod kedod changed the title Use memoized request_class value feat: Use memoized request_class value Mar 15, 2024
Copy link

codecov bot commented Mar 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.23%. Comparing base (d02ecbb) to head (2f9206e).

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #3205   +/-   ##
========================================
  Coverage    98.23%   98.23%           
========================================
  Files          321      321           
  Lines        14482    14488    +6     
  Branches      2302     2304    +2     
========================================
+ Hits         14227    14233    +6     
  Misses         113      113           
  Partials       142      142           

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

@kedod kedod force-pushed the use-memoized-request-class branch 3 times, most recently from e26b78d to a2856ab Compare March 15, 2024 00:44
Copy link
Member

@cofin cofin left a comment

Choose a reason for hiding this comment

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

LGTM

@cofin
Copy link
Member

cofin commented Mar 15, 2024

@all-contributors add @kedod for code

Copy link
Contributor

@cofin

@kedod already contributed before to code

Copy link
Contributor

@peterschutt peterschutt left a comment

Choose a reason for hiding this comment

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

Can we do resolve_response_class() at the same time? Its documented as being memoized just like this one, but isn't.

@peterschutt
Copy link
Contributor

For interests sake, the memoization of resolve_response_class() was removed in #357.

That PR added resolve_response_handler() which is itself cached and appears to be the only place that we call resolve_response_class() internally - so I can understand why its caching was removed in that case.

However, it has since always been documented as being cached, even though it isn't, and we offer it as public API, so if users are calling it more than once they'd benefit from the caching as documented and it wouldn't really cost us anything. So, now that we are caching resolve_request_class() I think its it probably better to memoize it too so the similar methods are consistent.

In that PR we also removed caching for resolve_response_cookies() and resolve_response_headers(), but the docstrings of those methods were updated to reflect that.

@kedod
Copy link
Contributor Author

kedod commented Mar 15, 2024

@peterschutt I discovered exactly the same thing last evening and removed the changes for the resolve_response_class.
But being consistent in the public APIs is a valid point.
I've added memoization on the resolve_response_class level.

@kedod kedod force-pushed the use-memoized-request-class branch from 8c3a6a4 to c4db439 Compare March 15, 2024 07:31
@kedod kedod changed the title feat: Use memoized request_class value feat: Use memoized request_class and response_class values Mar 15, 2024
@provinzkraut provinzkraut force-pushed the use-memoized-request-class branch from e5e2023 to 90a5ff5 Compare March 16, 2024 09:04
@provinzkraut provinzkraut enabled auto-merge (squash) March 16, 2024 09:05
@provinzkraut provinzkraut force-pushed the develop branch 2 times, most recently from 353ebca to 525cd4c Compare March 16, 2024 09:07
@provinzkraut provinzkraut force-pushed the use-memoized-request-class branch from 90a5ff5 to 2f9206e Compare March 16, 2024 09:13
Copy link

Documentation preview will be available shortly at https://litestar-org.github.io/litestar-docs-preview/3205

@peterschutt peterschutt disabled auto-merge March 16, 2024 09:22
@peterschutt peterschutt enabled auto-merge (squash) March 16, 2024 09:22
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.

4 participants