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

Halve the number of implementations of removeDotSegments #2207

Merged
merged 3 commits into from Oct 30, 2018

Conversation

Projects
None yet
3 participants
@aeons
Member

aeons commented Oct 25, 2018

I noticed that we had implementations of RFC3986's Remove Dot Segments in two places (and I think we removed another implementation already?).

I made a very quick JMH benchmark:

[info] Benchmark                           Mode  Cnt       Score       Error  Units
[info] ThroughputBenchmark.recursive      thrpt    3   97013.737 ±  5737.676  ops/s
[info] ThroughputBenchmark.stringBuilder  thrpt    3  183276.296 ± 23578.091  ops/s

And replaced the recursive implementation, with the string builder based one.

It now lives in UriFunctions only, which means it can be found as Uri.removeDotSegments.

@rossabaker

👍

@rossabaker

This comment has been minimized.

Member

rossabaker commented Oct 28, 2018

I'm assuming that the PathNormalizerSpec didn't add any interesting tests above and beyond what's already in UriSpec.

@aeons

This comment has been minimized.

Member

aeons commented Oct 28, 2018

They both had the same examples from the RFC.

@aeons

This comment has been minimized.

Member

aeons commented Oct 28, 2018

Ah wait, maybe the property test should stay.

@aeons aeons force-pushed the aeons:there-can-be-only-one branch from 3b39fa9 to 23eff79 Oct 28, 2018

@rossabaker

This comment has been minimized.

Member

rossabaker commented Oct 29, 2018

Needs a scalafmt. openjdk11 error is yet another instance of #2192.

@ChristopherDavenport

👍 after scalafmt.

@ChristopherDavenport ChristopherDavenport added this to the 0.20.0 milestone Oct 29, 2018

@ChristopherDavenport ChristopherDavenport merged commit b64b443 into http4s:master Oct 30, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@aeons aeons deleted the aeons:there-can-be-only-one branch Nov 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment