<br><br><div class="gmail_quote">On Thu, Mar 25, 2010 at 4:25 PM, Jeremy Shaw <span dir="ltr"><<a href="mailto:jeremy@n-heptane.com" target="_blank">jeremy@n-heptane.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<div>On Thu, Mar 25, 2010 at 12:29 PM, Michael Snoyman <span dir="ltr"><<a href="mailto:michael@snoyman.com" target="_blank">michael@snoyman.com</a>></span> wrote:<br></div><div class="gmail_quote"><div>
<blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
OK, here are my initial code comments:<div><br></div><div>* Do we want to move everything into Web.URLT? More to the point, I'm not sure I see the point of calling this URLT, since it doesn't really require any monad transformers; maybe we should call it web-routes and then the module would be Web.Routes?</div>
</blockquote><div><br></div></div><div>I think Web.Routes is a fine name. I'll make it happen. In the rest of this post I refer to things by the old names, but I do intend to change the module names and rename the package to web-routes.</div>
<div>
<div> </div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"><div>* I like the PathInfo class and to/fromPathSegments. Perhaps we should bundle that with the decode/encodePathInfo into a single module?</div>
</blockquote><div><br></div></div><div>I put PathInfo in a separate module because I am a little dubious of classes these days. I find it a bit annoying that you can only have one PathInfo instance per type. And I think it helps show that using PathInfo is not actually required. But, in practice, I think having less modules is probably a good thing in this case, since it does not affect the dependency chain at all. Just because I *can* put every function in it's own module doesn't mean I should. ;) Also, we probably do want people to provide PathInfo instances, even if they don't have to..</div>
<div>
<div><br></div></div></div></blockquote><div>I also am beginning to share a mistrust of classes; I think I went a little too overboard on them on a few previous packages (namely, convertible-text) and am now having a reaction in the opposite direction. I'm sure one day I'll find the Golden Path...<br>
<br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"><div class="gmail_quote"><div><div></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<div>* I'd like to minimize dependencies as much as possible for the basic package. The two dependencies I've noticed are Consumer and applicative-extras. I think the type signatures would be clearer *without* those packages included, eg:</div>
<div><br></div><div> fromPathSegments :: [String] -> Either ErrMsg a</div></blockquote><div><br></div></div><div>Except that is not a usable type. fromPathSegments may consume, some, but not all of the path segments. Consider the type:</div>
<div><br></div><div>data SiteURL = Foo Int Int</div><div><br></div><div>fromPathSegments is going to receive the path segments:</div><div><br></div><div>["Foo","1","2"]</div><div><br></div><div>
If you wrote a parser by hand, you would want it to look a little something like:</div><div><br></div><div> do string "Foo"</div><div> slash</div><div> i <- fromPathSegments</div><div> slash</div>
<div> j <- fromPathSegments</div><div> eol</div><div> return (Foo i j)</div><div> </div><div>The key concept here is that when you call fromPathSegments to get the first argument of Foo you need to know how many of the path segments were consumed / are remaining, so you can pass only those segments to the second fromPathSegments.</div>
<div><br></div><div>So you really need a type like:</div><div><br></div><div><div> fromPathSegments :: [String] -> (Either ErrMsg a, [String])</div><div><br></div><div>which outputs the unconsumed path segments.</div>
<div><br></div></div></div></blockquote><div>Well, given that as a criterion, I agree with the rest of your analysis entirely. However, I think we're looking at the purpose of fromPathSegments very differently. I'm not quite certain I understand why we would want to output the unconsumed segments; if something is unconsumed, then it seems like it's an invalid URL and should fail.<br>
<br>In your example, if I request "/Foo/5/6/7", fromPathSegments would return (Right (Foo 5 6), ["7"]); but what is going to consume that 7 now? The use case I envisioned for something like this is:<br>
<br>data BlogRoutes = ...<br>data MySite = MyHome | MyBlog BlogRoutes<br>fromPathSegments ("blog":rest) = MyBlog `fmap` fromPathSegments<br> <br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<div class="gmail_quote"><div><div></div><div>But this is obviously a ripe target for a monad of some sort -- trying keep track of the unconsumed portions by hand seems like it would asking for trouble... </div><div><br>
</div>
<div>The Consumer monad takes care of that and provides the functions you would expect such as, next, peek, and poke. And it seems nice to be able to use Monad, MonadPlus, Applicative, Alternative, etc, for composing fromPathSegments into larger parsers ?</div>
<div><br></div><div>But, perhaps there is a better choice of monad, or a better way of dealing with the problem? Or maybe it's not really a problem?</div><div><br></div><div>I think Failing is a pretty nifty data-type for dealing with errors. But perhaps it is not a big win here.. The #1 thing that makes Failing better than (Either [String] a) is it's Applicative instance. Specifically, Failing will accumulate and return all the errors which have occurred, not the just first failure (which is the behavior for Applicative (Either e)).</div>
<div><br></div><div>So for example, let's say you are doing are trying to lookup a bunch of keys from the query string. The key / value pairs in the query string are typically independent of each other. So let's say you do:</div>
<div><br></div><div> (,) <$> lookup "foo" <*> lookup "bar"</div><div><br></div><div>but neither of those keys exist. With Either you will only get the error 'could not find "foo"'. But with Failing you will get the error 'could not find "foo". could not find "bar"'. It is nice to get a report of all the things that are broken, instead of getting only one error at a time, fixing it, and then getting another error, etc.</div>
<div><br></div><div>However, I am not sure if this property is all that useful which urlt. If you are trying to parse a url like:</div><div><br></div><div> (string "Foo" *> Foo) <$> fromPathSegments <*> fromPathSegments</div>
<div><br></div><div>And the parsing of "Foo" fails.. then there is no use in finding out if the other segments parse ok -- because they are likely to be garbage. Maybe it failed because it got the string "FOo" instead of "Foo", but more likely it got something completely unrelated like, /bar/c/2.4.</div>
<div><br></div><div>So, perhaps Either is a better choice even with out considering dependencies... I think that Applicative / Alternative instances for Either are only defined in transformers in the Control.Monad.Error module -- which is a bit annoying. But we don't actually need those to implement urlt itself. </div>
<div><br></div><div>This brings up another detail though. </div><div><br></div><div>the fromPathSegments / Consumer stuff is basically implementing a parser. Except, unlike something like parsec, we do not keep track of the current position for reporting errors. I wonder if we should perhaps use a slightly richer parser environment. Within a web app, once you got your to/from instances debugged, you will never get a parse error, so having great error messages is not essential. But, for other people linking to your site it could be potentially helpful. Though, it seems like the current error messages out to be sufficient given how short the urls are..</div>
<div><br></div></div></div></blockquote><div>I don't think fancy error reporting will help here. More to the point: we could always layer a fancy parser on top of a simpler typeclass. For that matter, the same argument can be made for Failing and Consumer.<br>
<br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"><div class="gmail_quote"><div><div></div></div><div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<div>I'm not certain what exactly the type of ErrMsg should be here; I don't really have a problem using [String], which would be close to the definition of Failing.</div>
<div><br></div><div>* I think it's very important to allow users to supply customized 404 pages. Essentially, we need to augment handleWai (possibly others) with a (ErrMsg -> Application) parameter.</div></blockquote>
<div><br></div></div><div>Yeah, there are (at least) two possibilities, add an extra param for the handler. Or bubble the error up to the top:</div><div><br></div><div><div>handleWai_1 :: (url -> String) -> (String -> Failing url) -> String -> ([ErrorMsg] -> Application) -> ((url -> String) -> url -> Application) -> Application</div>
<div>handleWai_1 fromUrl toUrl approot handleError handler =</div><div> \request -></div><div> do let fUrl = toUrl $ stripOverlap approot $ S.unpack $ pathInfo request</div><div> case fUrl of</div><div> (Failure errs) -> handleError errs request</div>
<div> (Success url) -> handler (showString approot . fromUrl) url request</div><div> </div><div>handleWai_2 :: (url -> String) -> (String -> Failing url) -> String -> ((url -> String) -> url -> Application) -> (Request -> IO (Failing Response))</div>
<div>handleWai_2 fromUrl toUrl approot handler =</div><div> \request -></div><div> do let fUrl = toUrl $ stripOverlap approot $ S.unpack $ pathInfo request</div><div> case fUrl of</div><div> (Failure errs) -> return (Failure errs)</div>
<div> (Success url) -> fmap Success $ handler (showString approot . fromUrl) url request </div><div><br></div><div>The second choice is perhaps more flexible. Which do you prefer? In the first option, the handleError function could be a Maybe value -- and if you supply Nothing you get some default 404 page?</div>
<div><br></div></div></div></blockquote><div>I personally prefer the first option exactly as you describe it, but you're also correct that the second is more flexible. If anyone else reading this thread would prefer the second, speak now or forever hold your peace ;).<br>
<br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"><div class="gmail_quote"><div><div></div></div><div>In happstack we have a third possiblity. The ServerMonad is an instance of MonadPlus so we can throw out the error message and just call mzero:</div>
<div><br></div><div><div>implSite :: (Functor m, Monad m, MonadPlus m, ServerMonad m) => String -> FilePath -> Site url String (m a) -> m a</div>
<div>implSite domain approot siteSpec =</div><div> do r <- implSite_ domain approot siteSpec</div><div> case r of</div><div> (Failure _) -> mzero</div><div> (Success a) -> return a</div><div><br>
</div><div>implSite_ :: (Functor m, Monad m, MonadPlus m, ServerMonad m) => String -> FilePath -> Site url String (m a) -> m (Failing a)</div><div>implSite_ domain approot siteSpec =</div><div> dirs approot $ do rq <- askRq</div>
<div> let pathInfo = intercalate "/" (rqPaths rq)</div><div> f = runSite (domain ++ approot) siteSpec pathInfo</div><div> case f of</div>
<div> (Failure errs) -> return (Failure errs)</div><div> (Success sp) -> Success <$> (localRq (const $ rq { rqPaths = [] }) sp)</div><div><br></div><div>then we can do:</div>
<div><br></div><div> msum [ implSite "domain" "approot" siteSpec</div><div> , default404</div><div> ]</div><div><br></div><div>if implSite calls mzero, then the next handler (in this case default404) is tried. </div>
</div><div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<div>* It might be nice to have "type WaiSite url = Site url String Application". By the way, are you certain you want to allow parameterization over the pathInfo type?</div></blockquote><div><br></div></div><div>
I'm not certain I don't want to allow it... I have a vague notion that I might want to use Text sometimes instead of String. Though if I was really committed to that then I should make toPathInfo and fromPathInfo parameterized over pathInfo as well... So perhaps I will axe it from Site for now. I need to change the name of that type and it's record names too I think.</div>
<div>
<div> </div></div></div></blockquote><div>Referring to the fear of typeclasses mentioned above: I'd like to avoid MPTCs even more so. In fact, as I look at it, each extra parameter we add creates more potential for incompatible components. For instance, I can see an argument being made to use extensible exceptions for the fromPathSegments return type, but I'd rather keep things standard with [String] than create more division.<br>
<br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"><div class="gmail_quote"><div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<div>The only packages that I feel qualified to speak about then are urlt and urlt-wai, and my recommendation would be:</div>
<div><br></div><div>urlt contains decode/encodePathInfo, PathInfo class and related functions, Site and related functions. If you agree on allowing the parameterization of 404 errors, then also provide a default 404 error.</div>
</blockquote><div> </div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"><div>urlt-wai contains WaiSite, handleWai and related functions.</div>
</blockquote><div><br></div>
</div><div>Yeah, that is what I was thinking. urlt would contain what is currently in;</div><div><br></div><div>URLT.Base</div><div>URLT.PathInfo</div><div>URLT.HandleT</div><div>URLT.Monad</div><div>URLT.QuickCheck</div>
<div><br>
</div><div>QuickCheck module does not actually depend on QuickCheck, which is nice because QC1 vs QC2 is a big problem right now. </div><div><div> </div></div></div></blockquote><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<div class="gmail_quote"><div><div></div><div>It might also be nice to include:</div><div><br></div><div>
<a href="http://URLT.TH" target="_blank">URLT.TH</a> </div>
</div><div><br></div><div><div>with depends on template-haskell. But I am not sure that depending on template-haskell is an issue because template-haskell comes with ghc6, and the code in <a href="http://URLT.TH" target="_blank">URLT.TH</a> already handles the breakage that happened with TH 2.4.</div>
<div><br></div></div></div></blockquote><div>I have a different motive for keeping the TH code out: it seems like all of the other pieces of code should be relatively stable from early on, while the TH code (and quasi-quoting, and regular) will probably have some major changes happening for a while. It would be nice to have a consistent major release number for long periods of time on the core.<br>
<br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"><div class="gmail_quote"><div><div></div></div><div>If I switch to Either instead of Failing I believe the dependencies would be:</div>
<div><br></div><div> base, Consumer, template-haskell, network, utf8-string</div><div><br></div><div>urlt-wai would just include:</div>
<div><br></div><div>URLT.Wai</div><div><br></div></div></blockquote><div>Sounds great. Let me know when this is available for review. If you want me to do any of the merging/renaming, I have some time now (I arrived in southern California at 3:30 in the morning...).<br>
<br>Michael <br></div></div>