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 ValueStringBuilder adding the query parameters #1719

Merged
merged 3 commits into from
Jun 24, 2024

Conversation

TimothyMakkison
Copy link
Contributor

Uses ValueStringBuilder to generate query parameters for the uri and prepend a question mark.

Update: turns out UriBuilder has been changed between .NET Framework and Core. Core checks for leading ? symbols whereas Framework always adds one. I've added a preprocessor directive to resovle this issue.

Original

Method Mean Error StdDev Gen0 Gen1 Allocated
TaskTQueryLong_Async 12.97 us 0.258 us 0.665 us 1.5717 0.0458 14.45 KB
TaskTCollectionQuery_Async 18.64 us 0.372 us 0.848 us 1.8921 0.0610 17.4 KB

Changes

Method Mean Error StdDev Median Gen0 Gen1 Allocated
TaskTQueryLong_Async 12.81 us 0.278 us 0.712 us 12.60 us 1.5259 - 14.15 KB
TaskTCollectionQuery_Async 18.21 us 0.325 us 0.348 us 18.24 us 1.8311 0.0305 16.85 KB

@ChrisPulman
Copy link
Member

@TimothyMakkison there's a merge conflict, is it possible for you to solve this? I am in Dubai at the moment and the connection is very poor, once the conflict is solved, I will try to review this PR.

Copy link

codecov bot commented Jun 20, 2024

Codecov Report

Attention: Patch coverage is 32.84672% with 92 lines in your changes missing coverage. Please review.

Project coverage is 84.69%. Comparing base (6ebeda5) to head (5428291).
Report is 41 commits behind head on main.

Files Patch % Lines
Refit/ValueStringBuilder.cs 23.96% 88 Missing and 4 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1719      +/-   ##
==========================================
- Coverage   87.73%   84.69%   -3.04%     
==========================================
  Files          33       35       +2     
  Lines        2348     2483     +135     
  Branches      294      317      +23     
==========================================
+ Hits         2060     2103      +43     
- Misses        208      300      +92     
  Partials       80       80              

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

@TimothyMakkison
Copy link
Contributor Author

@TimothyMakkison there's a merge conflict, is it possible for you to solve this? I am in Dubai at the moment and the connection is very poor, once the conflict is solved

Should be up to date now and I've made a couple of readability improvements.

I will try to review this PR.

I'm in no rush to have this merged, please don't take time out of your holiday for this 😄

I see that #1389 is closed, as a maintainer do you think my proposal would be accepted? I'm pretty confident I can prevent any breaking changes while drastically improving startup times, but I'm not sure if the core team is interested. Otherwise I can continue improving the current library.

@ChrisPulman
Copy link
Member

I'm working here in Dubai unfortunately, but just until the end of this month, I still get to benefit from the good weather 🙂, I will make time over the weekend to go through a few things. I have made a release, hopefully nothing bad happened with it, I have seen one element that needs updating to resolve, but a fairly simple temporary fix for the user.

I need to create a good service and client combination to use as a live test, as not everything is tested and some things seem untestable due to the nature of the code.

@ChrisPulman ChrisPulman merged commit 2bf78ca into reactiveui:main Jun 24, 2024
1 of 3 checks passed
@TimothyMakkison TimothyMakkison deleted the optimize_query branch June 24, 2024 19:41
Copy link

github-actions bot commented Jul 9, 2024

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 9, 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