search

mirage / ocaml-uri

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

Fix #82, Empty path segments handled incorrectly #83

Closed moonlightdrive closed 9 years ago

moonlightdrive commented 9 years ago

Hi, hope this looks okay. Particularly the tests....

dsheets commented 9 years ago

Thanks! I'm surprised at the size of the fix. I've left a couple line comments but otherwise it looks really good.

moonlightdrive commented 9 years ago

I made the changes you suggested.

What's the reasoning behind not using the if statement? Is it just more pleasant/legible without "if", or is it something else?

avsm commented 9 years ago

Thanks! Both the fix and the test cases look good to me. There seems to be a strange failure in Yorick as regards use of |> though -- not sure what's going on there (or that it's related to this PR at all)

File "travis_opam.ml", line 146, characters 6-8:
Error: Unbound value |>
dsheets commented 9 years ago

This is due to ocaml/ocaml-travisci-skeleton#58 at https://github.com/ocaml/ocaml-travisci-skeleton/pull/58/files#diff-9846f191f8d66f21dfc85443e92c56edR146.

dsheets commented 9 years ago

There is no functional difference from removing the if -- it is a purely style issue to not nest conditionals inside of pattern matches. In particularly, it removes nesting (good) and makes it easier for compiler analysis to compile the pattern match (a sequence/tree of conditionals) efficiently.

moonlightdrive commented 9 years ago

Ah I see. Thank you.