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

Remove unnecessary mutating behavior from Parameters.getCatchall() #130

Merged
merged 2 commits into from
May 5, 2024

Conversation

gwynne
Copy link
Member

@gwynne gwynne commented May 5, 2024

These changes are now available in 4.9.1

Parameters.getCatchall() has historically been mutating, in order to delay the removal of percent encoding from matched catchall values until it is called for the first time. This was an unnecessary optimization which just adds confusion to the behavior of the API and makes it harder to use. We now remove the percent encoding immediately when setCatchall() is called instead.

Additional changes:

  • Minimum Swift version updated to 5.8
  • set(_:to:) now falls back to the original value if removing percent encoding fails, matching the existing behavior of the catchall logic. This should have no impact in real systems, as an invalid input of this kind should never reach the routing logic.

(N.B.: While it is an API break for a previously non-mutating method to become mutating, the reverse is not the case, so this is a patch-level change.)

gwynne added 2 commits May 5, 2024 08:25
…Catchall(), the same way it's done in set(). Also use the same fallback for percent decoding in set() that we do in the catchall.
@gwynne gwynne added bug Something isn't working semver-patch Internal changes only labels May 5, 2024
Copy link

codecov bot commented May 5, 2024

Codecov Report

Attention: Patch coverage is 87.50000% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 83.65%. Comparing base (2a92a7e) to head (410ee7b).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #130      +/-   ##
==========================================
- Coverage   83.76%   83.65%   -0.11%     
==========================================
  Files           4        4              
  Lines         234      208      -26     
==========================================
- Hits          196      174      -22     
+ Misses         38       34       -4     
Files Coverage Δ
Sources/RoutingKit/Parameters.swift 88.23% <87.50%> (+13.23%) ⬆️

... and 1 file with indirect coverage changes

Package@swift-5.9.swift Show resolved Hide resolved
Sources/RoutingKit/Parameters.swift Show resolved Hide resolved
@gwynne gwynne merged commit 8c9a227 into main May 5, 2024
10 of 11 checks passed
@gwynne gwynne deleted the fix-parameters-catchall branch May 5, 2024 21:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working semver-patch Internal changes only
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants