domkm / silk

Routing for Clojure & ClojureScript
222 stars 13 forks source link

Encode/decode as a protocol #12

Closed whodidthis closed 9 years ago

whodidthis commented 9 years ago

One way to enable people to use whatever encoding by extending a protocol (#11).

domkm commented 9 years ago

I'm not sure how this would work because protocols dispatch on type and we're only encoding/decoding strings. Could you clarify the use cases for these protocols?

whodidthis commented 9 years ago

Really I only want to override the default encoding. So basically instead of

(ns ... (:require [domkm.silk :as silk]))
(def my-routes (silk/routes [[:r [["find" :thing]]]]))
(silk/depart my-routes :r {:thing "나이스"}) ;=> "/find/%EB%82%98%EC%9D%B4%EC%8A%A4"

I want to not use percent encoding on my web page since its ok for me browser support wise

(ns ... (:require [domkm.silk :as silk]))
(extend-protocol silk/EncodeURI
  String
  (encode [this] this))
(def my-routes (silk/routes [[:r [["find" :thing]]]]))
(silk/depart my-routes :r {:thing "나이스"}) ;=> "/find/나이스"
domkm commented 9 years ago

Okay, thanks for clarifying. I don't think a protocol is the right thing to use for this. I'd prefer passing encode/decode functions as options or having the encode/decode vars be dynamic.

However, seeing as this will be a non-issue in the next major release, I'm going to decline this feature for now since I suspect it will break any parts of the current implementation that assume strings are always decoded.

Thanks for using Silk, providing feedback, and submitting this PR! Hopefully you can temporarily work around the automatic encoding/decoding issue. In retrospect, I think that this feature was a poor decision on my part.

whodidthis commented 9 years ago

This was indeed a bit of a random workaround, great to hear it will be available in the future