New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Re-implement Muuntaja as Protocol #60
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's so much code moving around that this is pretty hard to review, but it mostly looks okay. The UTF-8 optimization makes sense to me, it should be the most common path!
src/clj/muuntaja/parse.clj
Outdated
(.clear cache)) | ||
(.putIfAbsent cache args ret)) | ||
ret))))) | ||
(if-not (= args empty) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what this is? Always return nil if for ((fast-memoize .. f) nil)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch. we need to either guard against input to the memoize or handle the nil-case within it. Without this, the ((fast-memoize .. f) nil)
always misses the cache ('(nil)
is not found on the cache as it returns nil
and the .get
returns nil
. Not sure why. But the execution cost goes from ~30ns to 3000ns. For cases like "no accept-header set" etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about using .getOrDefault
?
(let [cache ...
sentinel (Object.)]
(fn [& args]
(let [cached (.getOrDefault cache args sentinel)]
(if (identical? sentinel cached)
(comment compute the value)
cached))
src/clj/muuntaja/core.clj
Outdated
{:type type | ||
:format format} | ||
e))))) | ||
(defn consumes [m] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All these small public functions would benefit from docstrings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indeed.
Since this removes the Muuntaja record, should probably bump the version to |
thans for the review, fixed those. |
negotiate-and-format-request
(less hash-lookups)