Other

[cryogenian]

I've tried to use

newtype RoutingError = RoutingError [[String]]

as an error, but first attempt have been very ugly, so I've revert it to String.

matches fires when newURL can be parsed, if oldURL can not be parsed then it fires with Nothing as first param.

[jdegoes]

Review by @paf31 / @garyb

[paf31]

I think it looks good. I can spend more time on this tonight and this weekend, but for now:

• I would strongly recommend not using Monad if you can help it. Even adding num and bool to the Match class would probably be preferable, so that it's possible to perform analyses and do documentation generation.
• I'll spend some time thinking about the representation of errors.

[paf31]

If we can't make errors into a semiring, maybe there's a middle ground where we introduce an ADT for single errors and use a list of errors to represent multiple failures in parallel.

One last comment: consider using List instead of Array.

[cryogenian]

Why List?

Errors are working perfectly as semiring. I switched to String because of needs of handling those errors in state or something like state. Why do we need all those errors not only first or last?

[paf31]

• Cons patterns are being deprecated in the compiler, so it's better to switch to lists now.
• You can always pretty print errors, but you don't want to have to parse them to match certain classes of errors in the UI. Also, it's useful to keep a list so that you can present a useful error. Consider for example the route Foo <$> (lit "foo" *> var) <|> Bar <$> (lit "Bar" *> var). if the user mistypes #/bam/123 they will get the error "expected foo", but a better error is "expected foo or bar".

[paf31]

This representation doesn't seem to represent the fact that you only have one Query part. Why not

data Route = Route { parts :: List String, params :: M.StrMap String }

[cryogenian]

foo/?bar=12/baz/?quux=123/ is not equal to foo/?bar=12&quux=123/baz/.

[paf31]

Oops, sorry, I didn't realize you needed to support those cases. Isn't it more common to push the query parameters to the end?

[cryogenian]

Adding support of multiple queries is easy, and it can be useful sometimes.
I.e. in producing parameterized ajax and filtering grid with many filters simultaneously.

[paf31]

Oh, I'm not suggesting that there only be one query parameter. I just don't see the appeal of distinguishing foo/?bar=12/baz/?quux=123/ from foo/?bar=12&quux=123/baz/ as routes. I would normally expect query parameters to be put at the end, and for both of these to fail to parse:

foo/baz?bar=12&quux=123

[cryogenian]

I understand. But I believe that ability of matching routes with many query parts is useful

[paf31]

Here is my suggestion for the representation of errors:

https://gist.github.com/paf31/c4b5c06fab182073d533

It is the free semiring generated by a type. You can try it in psci as follows:

> free 1
(Free [[1]])

> free 1 + free 2
(Free [[1],[2]])

> (free 1 + free 2) * free 3
(Free [[1,3],[2,3]])

> (free 1 + free 2) * (free 3 + free 4)
(Free [[1,3],[1,4],[2,3],[2,4]])

> (free 1 + free 2) * (free 3 + free 4) * (free 5 + free 6)
(Free [[1,3,5],[1,3,6],[1,4,5],[1,4,6],[2,3,5],[2,3,6],[2,4,5],[2,4,6]])

> liftFree id $ (free 1 + free 2) * (free 3 + free 4) * (free 5 + free 6)
231

If you choose a base type MatchError for errors (I recommend creating an error ADT, not String) then you can use Free MatchError as your error type when matching. The SemiRing instance will take care of reordering things for you, and then you can use liftFree to render your errors in a readable way.

[cryogenian]

@paf31 thank you free semiring is awesome :)
I've added module Data.Validation.Alt with V datatype. It's almost similar with V from purescript-validation. Will it be useful to make pr to it? (i.e. move Data.Validation to Data.Validation.Apply and add Data.Validation.Alt)

[jdegoes]

Looking pretty good -- I agree the validation and free semiring stuff probably belongs elsewhere, but if we can't get those out now quickly, we can always do it after the fact. Any other comments / suggestions from @paf31 or @garyb? Is this ready to merge, @cryogenian??

[paf31]

Looks good to me, although a few more doc comments would probably help. I'd definitely take PRs on purescript-validation if you wanted to add something like Data.Validation.Semiring. I'm not sure where the free semiring stuff would go. Maybe in monoids, or maybe we should add a semirings package?

[jdegoes]

Yeah, I'd say add a purescript-semirings package in core.

[garyb]

👍 from me.

A purescript-semiring(s) package sounds good to me - we can move the Semiring class into that eventually too.

[cryogenian]

great! I'll make pr Data.Validation.Semiring purescript-validation and then it will be ready

[paf31]

I just created this, in case you want to send a PR. If not, we can leave it for later.
https://github.com/purescript/purescript-semirings

[jdegoes]

K, please also do a PR to semirings, it will add a small delay but hopefully @paf31 can approve & tag a version quickly.

[cryogenian]

I think it's ready.

(Maybe I made something wrong in purescript-semiring but it's not appeared in bower search)

[paf31]

Oops, my fault.

[paf31]

It should be there now.

[cryogenian]

Done

[jdegoes]

Let's just add some docs & examples in a separate PR.