mirage / ocaml-uri

RFC3986 URI parsing library for OCaml
Other
97 stars 57 forks source link

ADD Support modifying pct encoding safe and unsafe chars #146

Closed orbitz closed 3 years ago

orbitz commented 4 years ago

This is useful for reusing the pct encoding function but with custom character sets. For example, RFC 6570 uses some non-standard pct encodings in some template expansions.

orbitz commented 4 years ago

@avsm Does this seem reasonable?

avsm commented 4 years ago

Looks reasonable, but I'm not sure if it's better to allow also overriding other components (e.g. for non-compliant APIs, which seem quite commonplace). e.g. #145

orbitz commented 4 years ago

Yeah I don't think this solution works for #145. What do you think such an API would look like to solve that? Something like a pct_encoder type where you can specify the safe chars for each type and you can pass in such an encoding in to_string?

orbitz commented 4 years ago

I think what's in this PR could be the beginning of that solution. We could have a let make_pct_encoder ?(path =Path) ?(query_key = Query_key) ... = { path; query_key; ... }

And then users could use the custom component that this PR introduces, and then of_string could take the encoder and use the value in its uses of pct_encode

avsm commented 4 years ago

That sounds like a very good approach to me, and it would keep the homogenous interface. We'd need a to_string analogue as well though, wouldn't we?

avsm commented 4 years ago

Ah, I'm sorry - I skipped #142 which redoes the URI parsing entirely. If you could base your PR over that, I'd appreciate it, as I'd like to merge that soon.

orbitz commented 4 years ago

Whoops yes you're right, I meant to_string in my comment but wrote the wrong one. I don't think we need this for of_string.

I'll rebase my change on #142.

orbitz commented 4 years ago

Rebased, I'll look into adding the pct_encoder we talked about. Do you want that before the next release or post? And do you have a timeline for when you'd like the next release?

orbitz commented 4 years ago

Looks like this is failing, although I believe it is from @anmonteiro 's change.