-
Notifications
You must be signed in to change notification settings - Fork 2
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
Add uri_encode function (#273) #285
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added one inline comment on the encoding implementation.
We'd also have to add documentation on the new function in the README.
}; | ||
|
||
private static final PercentEscaper PERCENT_ESCAPER = new PercentEscaper("", false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I understood, the conclusion from #273 was to basically use the metamorph implementation, which uses java.net.URLEncoder
. If there is no specific reason for using PercentEscaper
here, I think using the standard Java class would be preferable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
java.net.URLEncoder
does not conform to RFC 3986.
Also, we want to conform to catmandu fix' uri_encode
. Perl does it right , see https://metacpan.org/pod/URI::Encode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In lobid-resources I added a UrlEscaper which also complies to RFC 3986, using an older com.google.gdata.util.common.base.PercentEscaper
. It hasn't make it to metafacture (yet).
In general, I would prefer to use the Metamorph implementation directly (see e.g. #230 and #231). If it doesn't provide a suitable interface it should be added there and then reused for both. Eventually, |
good point. Agreed. Awaiting ok for using percent encoder and then migrate function to metafcature-core.
Right - forgot about that. Would do this also im metfacture-core. Will note the RFC 3986 compliance there. |
One thing leading to confusion here (at least for me) is that we actually need something to encode a string to be used in a URI (in particular as a path segment), not to encode a full URI. So for Now if I understand it correctly, both metamorph.functions.URLEncode and Catmandu::Fix::uri_encode basically do that, but Java's To both avoid breaking the current |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. For increased flexibility the safeChars
option should be exposed to Fix and Morph.
This PR is substituted by #314. |
Resolves #273.