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: refactors, cache attributes, use helper methods #1739

Merged
merged 1 commit into from
Jun 26, 2024

Conversation

TimothyMakkison
Copy link
Contributor

@TimothyMakkison TimothyMakkison commented Jun 25, 2024

  • Replace BodylessMethods HashSet<HttpMethod> with IsBodyless function.
  • Prevent headerCollection from allocating a new dictionary if the value isn't a dictionary.
    • This shouldn't be possible, I'm not sure if this should be an error.
  • Use a default static QueryAttribute if queryAttribute is null.
  • Use a default static Uri in UriBuilder.
  • Cache QueryUriFormatAttribute in RestMethodInfo instead of every http request.
  • Use a switch expression for collectionFormat.

Sorry about the hundreds of prs, I did a code review a while back and am slowly getting through my list of suggestions. I don't want to create one giant hard to follow pr.

Overall saved 250 bytes.

Original

Method Mean Error StdDev Median Gen0 Gen1 Allocated
ConstantRouteAsync 9.987 us 0.5054 us 1.4003 us 9.826 us 0.7477 - 6.95 KB
DynamicRouteAsync 7.167 us 0.5555 us 1.5394 us 7.341 us 0.7858 0.0076 7.27 KB
ComplexDynamicRouteAsync 7.171 us 0.1430 us 0.2578 us 7.069 us 0.8545 - 7.9 KB
ObjectRequestAsync 9.756 us 0.7046 us 1.9757 us 8.828 us 0.9460 - 8.76 KB
ComplexRequestAsync 22.211 us 0.7385 us 2.0339 us 21.411 us 1.5869 - 14.8 KB

Changes

Method Mean Error StdDev Median Gen0 Gen1 Allocated
ConstantRouteAsync 9.188 us 0.9562 us 2.7436 us 8.393 us 0.7324 0.0076 6.73 KB
DynamicRouteAsync 4.738 us 0.0983 us 0.2692 us 4.662 us 0.7629 - 7.06 KB
ComplexDynamicRouteAsync 6.993 us 0.1396 us 0.3750 us 6.872 us 0.8316 - 7.69 KB
ObjectRequestAsync 7.909 us 0.1558 us 0.2964 us 7.782 us 0.9155 - 8.5 KB
ComplexRequestAsync 23.940 us 1.0756 us 2.9985 us 23.497 us 1.5564 - 14.57 KB

Copy link

codecov bot commented Jun 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.62%. Comparing base (6ebeda5) to head (4717574).
Report is 48 commits behind head on main.

Current head 4717574 differs from pull request most recent head 1c8bee8

Please upload reports for the commit 1c8bee8 to get more accurate results.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1739      +/-   ##
==========================================
- Coverage   87.73%   84.62%   -3.11%     
==========================================
  Files          33       36       +3     
  Lines        2348     2511     +163     
  Branches      294      328      +34     
==========================================
+ Hits         2060     2125      +65     
- Misses        208      304      +96     
- Partials       80       82       +2     

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

@TimothyMakkison TimothyMakkison force-pushed the refactor branch 2 times, most recently from de82237 to 4717574 Compare June 26, 2024 10:07
@ChrisPulman
Copy link
Member

@TimothyMakkison This has a merge conflict, could you solve please, thank you

@ChrisPulman ChrisPulman merged commit 12640cb into reactiveui:main Jun 26, 2024
1 check passed
@TimothyMakkison
Copy link
Contributor Author

TimothyMakkison commented Jun 26, 2024

Mb, thought I'd solved them. Give me 5 mins

Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants