Skip to content

Conversation

@guymers
Copy link
Contributor

@guymers guymers commented Dec 10, 2017

Use an RFC3986 compliant encoder instead of URLEncoder.encode() when
encoding URI path segments.

Use an RFC3986 compliant encoder instead of URLEncoder.encode() when
encoding URI path segments.
@pchlupacek
Copy link
Member

hi @guymers could you please be so kind and provide more details about this PR? I just want to be sure we follow correctly what does this solve, so we could add this to next build.

Thanks a lot.

@guymers
Copy link
Contributor Author

guymers commented Feb 2, 2018

I ran into a problem when using the fs2-http client to talk to Chromium. Chromium 63 changed the id to be surrounded by brackets. If path segments are encoded with URLEncoder.encode then urls turn into /json/close/%28ABE15FB09437A38EE95AB3377E8A42EB%29 which Chromium rejects: (404): No such target id: %28E87469E8F3F90CA5DBF2C8214439C68%29. Changing the path segment encoding to be RFC3986 compliant removed this error.

@AdamChlupacek
Copy link
Member

Since we say our URI based on 3986 I think this is valid change to be included.

@AdamChlupacek
Copy link
Member

@guymers Do you think we could change the encoder to be actually scodec.Codec ? so that it can play better with the whole ecosystem of protocols

@guymers
Copy link
Contributor Author

guymers commented Feb 4, 2018

I don't know. I haven't used scodec.Codecthat much and don't have any spare time to look into it at the moment.

@AdamChlupacek
Copy link
Member

@guymers I can do it, if you are ok with me using your code?

@guymers
Copy link
Contributor Author

guymers commented Feb 4, 2018

Sure, you can do whatever you want with it.

Copy link
Member

@AdamChlupacek AdamChlupacek left a comment

Choose a reason for hiding this comment

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

Actually never mind, by long reach it is used in codecs how it should be. We will not get any thing out of refactoring this. Great job! Thank you for contributing.

@AdamChlupacek AdamChlupacek merged commit 6da4d6d into Spinoco:series/0.3 Feb 5, 2018
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.

3 participants