Add with_uri

[hcarty]

Addresses #96 and possibly #89

I can add tests if the approach and naming are satisfactory.

[dsheets]

Unfortunately, with_path has the signature val with_path : t -> string -> t. Could you please modify the function to take this into account and then add tests?

Perhaps the name should simply be with_ rather than with_uri?

[dbuenzli]

Unfortunately, with_path has the signature val with_path : t -> string -> t. Could you please modify the function to take this into account

I'm not sure what you meant by this but it seems to me that the signature of the PR is the right way to handle optional args, otherwise you cannot erase them and always have to specify them.

# let f ?x ?y t = t;;
val f : ?x:'a -> ?y:'b -> 'c -> 'c = <fun>                                      
# f 3;;
- : int = 3                                                                     
# let f t ?x ?y = t;;
val f : 'a -> ?x:'b -> ?y:'c -> 'a = <fun>                                      
# f 3;;
- : ?x:'_a -> ?y:'_b -> int = <fun>  

Perhaps the name should simply be with_ rather than with_uri?

Personally I prefer with_uri.

[dsheets]

Sorry I wasn't clearer. I agree with the function signature and it is correct. The issue is that the signature of the with_path function is not parallel to the other with_* functions and so the function body needs modification to account for this (and fix the CI type error).

I prefer with_ over with_uri exactly to avoid more like the above parallel signature/naming failure. There are a number of functions named like Uri.with_host and Uri.with_path so applications of this function would appear as Uri.with_ ~path:(Some "/foo") ~fragment:(Some "bar") which seems more parallel to me than Uri.with_uri ~path:(Some "/foo") ~fragment:(Some "bar").

[dbuenzli]

this function would appear as Uri.with_ ~path:(Some "/foo") ~fragment:(Some "bar")

I see the point, but from a code reading perspective you may easily miss the final u the result is based on. Having with_uri makes it clearer that an uri is being updated here.

[dsheets]

I'm sorry, I don't understand

you may easily miss the final u the result with parameter and hence not understand

Do you mean that the URI to be modified may be missed? Or the underscore is small?

[dbuenzli]

Do you mean that the URI to be modified may be missed? Or the underscore is small?

I mean that from a code reading perspective Uri.with_uri makes it much more clear that we are modifying an existing uri than Uri.with_ which could be mistaken for a constructor if you are not familiar with the module.

[dsheets]

Ah, that makes sense to me. Thanks for the justification, Daniel. I'm fine to stick with with_uri as it is definitely more explicit about not being a constructor.

[hcarty]

It'll be a day or two before I get a chance to come back to this and write tests most likely - but for now is updated implementation ok (barring proof otherwise with tests)? Specifically the local with_path_opt and with_query_opt functions.

[rgrinberg]

@dsheets @hcarty any updates on this? This is quite useful

[hcarty]

@rgrinberg I had lost track of this! Thanks for the ping. I'll try to get tests together after Compose NYC.

[hcarty]

@rgrinberg @dsheets Rebased and tests added

[hcarty]

The travis failure looks like an unrelated issue (couldn't retrieve a dependency's source for one of the builds).

[dsheets]

LGTM. Thanks!

[hcarty]

Thanks for your patience!