refit icon indicating copy to clipboard operation
refit copied to clipboard

feat: custom query key formatters

Open tcortega opened this issue 2 years ago • 4 comments

What kind of change does this PR introduce?

Feature and docs update.

What is the current behavior?

Query keys in the URL are formatted based on property names without any customization.

What is the new behavior?

Introduces the capability for users to define custom formatting strategies for query keys in the URL. This can be achieved by implementing the IUrlParameterKeyFormatter interface and setting it in RefitSettings. The default behavior remains unchanged (using property names as they are) to ensure backward compatibility. Also, the documentation has been updated to reflect these changes and guide users on how to utilize this feature.

What might this PR break? Care has been taken to ensure retro-compatibility. This PR should not introduce any breaking changes, as the default behavior remains consistent with previous versions.

Please check if the PR fulfills these requirements

  • [x] Tests for the changes have been added (for bug fixes / features)
  • [x] Docs have been added / updated (for bug fixes / features)

Other information:

  • The AliasAs attribute always takes precedence in key naming, regardless of any custom key formatters in use.
  • Enhanced documentation has been added to clarify the behavior of IUrlParameterFormatter when dealing with dictionaries.
  • The current handling of dictionary formatting remains unchanged to ensure backward compatibility and prevent potential breaks for existing users.
  • The decision to prioritize the AliasAs attribute aligns with intuitive expectations and mirrors behaviors such as JsonPropertyName in System.Text.Json.
  • Formatters are designed to be straightforward: they accept a string and return a formatted version without considering other attributes or extraneous factors.

tcortega avatar Sep 27 '23 20:09 tcortega

Closes #1333 Closes #1541 Closes #1258

There are probably more issues regarding this matter.

tcortega avatar Sep 27 '23 20:09 tcortega

Can this be looked into, or is the PR stale?

Thepriestdude avatar May 29 '24 15:05 Thepriestdude

Can this be looked into, or is the PR stale?

I guess they're too busy

tcortega avatar May 29 '24 15:05 tcortega

I was looking at this at the weekend, but I was away in Scotland, I working my way through the PR's to get them all dealt with appropriately. I will try to find time this weekend to go through this

ChrisPulman avatar May 29 '24 17:05 ChrisPulman

Codecov Report

Attention: Patch coverage is 87.90698% with 26 lines in your changes missing coverage. Please review.

Project coverage is 86.19%. Comparing base (6ebeda5) to head (51cc40b). Report is 29 commits behind head on main.

:exclamation: Current head 51cc40b differs from pull request most recent head 6913370

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

Files Patch % Lines
Refit/RequestBuilderImplementation.cs 91.19% 5 Missing and 9 partials :warning:
Refit/CamelCaseUrlParameterKeyFormatter.cs 55.00% 6 Missing and 3 partials :warning:
Refit/RefitSettings.cs 91.66% 2 Missing and 1 partial :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1570      +/-   ##
==========================================
- Coverage   87.73%   86.19%   -1.54%     
==========================================
  Files          33       34       +1     
  Lines        2348     2072     -276     
  Branches      294      297       +3     
==========================================
- Hits         2060     1786     -274     
+ Misses        208      206       -2     
  Partials       80       80              

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Jun 01 '24 22:06 codecov[bot]

@tcortega Please could you recheck the code you have added after the merge to be sure it still covers the same intended update

ChrisPulman avatar Jun 01 '24 22:06 ChrisPulman

@tcortega Please could you recheck the code you have added after the merge to be sure it still covers the same intended update

I'll check it either later tonight or tomorrow

tcortega avatar Jun 01 '24 22:06 tcortega

@ChrisPulman From my end, based on my changes, this is pretty much what I can do testing-wise.

tcortega avatar Jun 04 '24 05:06 tcortega

It looks like your unintentionally reverted #1619 when merging to the main branch, do you want me to fix this?

TimothyMakkison avatar Jun 09 '24 16:06 TimothyMakkison

@TimothyMakkison yes please

ChrisPulman avatar Jun 09 '24 17:06 ChrisPulman

@TimothyMakkison Trying to check everything as best as possible after the merges and updates to ensure all is still intact ready for a release, thank you for spotting this.

ChrisPulman avatar Jun 09 '24 17:06 ChrisPulman

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[bot] avatar Jun 24 '24 00:06 github-actions[bot]