Skip to content
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

Add key-fn library wide #68

Merged
merged 1 commit into from
Oct 18, 2020
Merged

Conversation

CHNB128
Copy link

@CHNB128 CHNB128 commented Oct 13, 2020

This PR attempt to solve these problems: #62 #51

Copy link
Member

@weavejester weavejester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall good, some minor formatting/documentation changes needed. Also we can squash this down into 1 commit - I don't think we need so much fidelity for a relatively small change.

@@ -46,6 +46,7 @@

Accepts the following options:

:key-fn - fn that will be applied to each key
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use "function" in the docs instead of "fn".

(if valid? (assoc-json-params request json))
[request options]
(if-let [[valid? json] (read-json request options)]
(when valid? (assoc-json-params request json))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't change lines unrelated to the changes in the PR.

@@ -130,6 +132,8 @@

Accepts the following options:

:date-format - an optional date format string that Date objects will be encoded with
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line shouldn't be here, right?

(let [request {:headers {"content-type" "application/json; charset=UTF-8"}
:body (string-input-stream "{\"foo\": \"bar\"}")
:params {"id" 3}}
handler (wrap-json-params identity {:key-fn (fn [k] (.toUpperCase (name k)))})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Align let values.

@weavejester weavejester merged commit f2facdf into ring-clojure:master Oct 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants