evancz / url-parser

Parse URLs into nicely structured data
http://package.elm-lang.org/packages/evancz/url-parser/latest/
BSD 3-Clause "New" or "Revised" License
114 stars 29 forks source link

Behaviour of "string" parser does not match example in docs #42

Closed ktonon closed 5 years ago

ktonon commented 7 years ago

Doc for function (</>) shows:

    parsePath (s "search" </> string) location
    -- /search/cats/  ==>  Just "cats"
    -- /search/frog   ==>  Just "frog"
    -- /search/       ==>  Nothing
    -- /cats/         ==>  Nothing

But parsePath (s "search" </> string) location returns Just "" because the implementation of string is

string : Parser (String -> a) a
string =
    custom "STRING" Ok

Not sure which is correct. Assuming the docs are the desired behaviour:

{-| Parse a segment of the path as a `String`.
    parsePath string location
    -- /alice/  ==>  Just "alice"
    -- /bob     ==>  Just "bob"
    -- /42/     ==>  Just "42"
    -- /        ==> Nothing
-}
string : Parser (String -> a) a
string =
    custom "STRING" <|
        \segment ->
            if String.isEmpty segment then
                Err "string does not match empty segment"
            else
                Ok segment

Using elm-doc-test would catch this earlier.

Caught the issue while working on a PR to add doc tests. Using elm 0.18.0

process-bot commented 7 years ago

Thanks for the issue! Make sure it satisfies this checklist. My human colleagues will appreciate it!

Here is what to expect next, and if anyone wants to comment, keep these things in mind.

crypticmind commented 6 years ago

Just stumbled upon this issue

crypticmind commented 6 years ago

The slash is required though. This is what I tested:

> import UrlParser exposing(..)
> location = { href = "", host = "", hostname = "", protocol = "", origin = "", port_ = "", pathname = "", 
> parsePath (s "aaa" </> string) { location | pathname = "/aaa/bbb" }
Just "bbb" : Maybe.Maybe String
> parsePath (s "aaa" </> string) { location | pathname = "/aaa/" }
Just "" : Maybe.Maybe String
> parsePath (s "aaa" </> string) { location | pathname = "/aaa" }
Nothing : Maybe.Maybe String
sethlivingston commented 6 years ago

Stumbled on this, too. @crypticmind do you have a PR?

crypticmind commented 6 years ago

@sethlivingston I created #53, but @ktonon also created #43. Either of them will fix the issue. In the meantime, just use your own string parser in your routes:

string : Parser (String -> a) a
string =
  custom "STRING" <|
    \segment ->
      if String.isEmpty segment then
        Err "Empty string"
      else
        Ok segment

In my code, I'm actually validating the string isn't also blanks:

string : Parser (String -> a) a
string =
  custom "STRING" <|
    \segment ->
      if segment |> String.trim |> String.isEmpty then
        Err "Empty or blank string"
      else
        Ok segment
ktonon commented 5 years ago

I'm closing this issue as this project is no longer active.