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

Support path prefixes #3

Merged
merged 1 commit into from
Jul 7, 2024
Merged

Conversation

kubukoz
Copy link
Contributor

@kubukoz kubukoz commented Jul 6, 2024

Closes #2

@@ -233,7 +233,14 @@ private[smithy4s_fetch] case class SimpleRestJsonCodecs(
uriScheme,
uri.host,
uri.port.toIntOption,
uri.pathname.split("/"),
uri.pathname
// drop the guaranteed leading slash, so that we don't produce an empty segment for it
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I think the "guarantee" is not strong enough for me - it's not a NonEmptyList after all.

I think it's better to do url.pathname.split("/").dropWhile(_.isEmpty) - it should be safer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took the guarantee from the Scaladoc, https://developer.mozilla.org/en-US/docs/Web/API/URL/pathname also repeats this claim... but yeah I can still add that

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually, dropWhile will drop empty segments if they precede non-empty ones, I think that'll break a test or two. Having a guaranteed slash before each segment is one way to ensure we don't accidentally drop segments. The spec seems pretty unambiguous on that part:

For each segment of url’s path: append U+002F (/) followed by segment to output

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, I sort of hoped empty segments were disallowed in general 🙃

Copy link
Contributor

@keynmol keynmol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot, especially for the tests!

@keynmol keynmol merged commit aa8c6a6 into neandertech:main Jul 7, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Double slash if the base URI has a path
2 participants