Fixes #7

[Deraen]

Hi,

I took a stab at fixing #7. I'm not perfectly happy how depart works with default parameters as it writes the default parameter to the url.

Decode-query now returns empty map for empty string. This way query string pattern defaults should work even if url has no query params at all. This shouldn't be problem as encode-url returns an empty query-string for empty map.

[Deraen]

I think using e.g. {:q nil} as a default should also be supported, but this doesn't allow that.

[domkm]

Thanks, Juho! I really appreciate you providing a patch for this and for explaining the tradeoffs.

Since @geraldodev, @leblowl, and @sritchie have run into this issue as well, I'd like to hear their thoughts on this patch. Would it be problematic if depart returned a URL with default params or if nil was an invalid default param value?

[Deraen]

Hi,

I've been thinking about this a bit more since Friday and I think supporting nil as valid default param would be quite important. I would perhaps even make the second param of ? optional.

(def routes (silk/routes {:search [["search"] {"q" (silk/? :q)}]}

(silk/arrive "/search") => {:name :search}
(silk/arrive "/search?q=foo") => {:name :search :q "foo"}

(silk/depart :search) => "/search"
(silk/depart :search {:q "foo"}) => "/search?q=foo"

The current solution to this search route requires me to have two routes: :search and :search-q as I couldn't get depart working with two routes with the same name. And wrapping the query params map in ? would require me to provide non-nil default.

[sritchie]

I love the idea of supporting nil as a valid default and allowing an optional second param. +1.

[Deraen]

I'll try to update my branch tomorrow with tests for allowing nil etc. and I'll see if I can get the implementation working.

[Deraen]

Okay,

I added some tests and a possible implementation. But I think this should probably be improved still: Now the ? implementation is too specific to maps and it probably shouldn't be?

[sritchie]

this API looks good to me. I'd love to try this out, if @DomKM is willing to publish a snapshot. Thanks again!

[domkm]

Absolutely! Quick question for @Deraen first, what do you mean that this implementation of ? is too specific to maps? Will this patch break any uses of ? that work with the current implementation?

[Deraen]

I'm not exactly sure of the all the uses of ?, the tests seem to currently use it only with maps. I don't think this breaks any uses but if default-params is not provided to the ? the implementation will create a map:

• match: (or default-params {ptrn nil}
• unmatch: (or default-params {ptrn ::optional-key-has-no-value})

The defaults make sense when ptrn is keyword, but for e.g. regexPattern not so much:

user=> (silk/match {"a" (silk/? (silk/int :a))} {})
{#domkm.silk.RegexPattern{:param-key :a, :regex #"\d+", :deserialize #<silk$int$fn__14777 domkm.silk$int$fn__14777@7e0ace58>, :serialize #<core$str clojure.core$str@1988a9c1>, :validate #<core$integer_QMARK_ clojure.core$integer_QMARK_@6238c6f9>} nil}

[Deraen]

I can think of several ways to improve this:

• Currently sub patterns of ? don't know that they are being used from inside ?. This means that ? needs to provide them with default params if there is no real parameter. Pattern protocol could be extended with a new method, create-default or such which ? can use to create defaults for different kinds of patterns
• ? sets a dynamic var and each other Pattern handles how they work if they are marked as optional
• The API is changed so that ? pattern is not used to wrap other patterns, but is used to look up optional values from route: clj (silk/match {"a" (silk/int (silk/? :a))} {})
  • Might not be feasible
  • Probably each other Pattern would need to deal with what to do if sub Pattern didn't have value but was optional (probably using special value like currently).

Probably the first option would be the simplest (extending the current implementation to work with other Patterns). Does this make any sense?

[domkm]

Thanks for your work on this, @Deraen!

I just cut 0.1.0-SNAPSHOT. Feedback greatly appreciated. CC @sritchie @leblowl @geraldodev

[sritchie]

This fixes my bug. Thanks for the work on this, guys.