r0man / oauth-clj

Clojure OAuth library
95 stars 27 forks source link

Hyphenize body #9

Closed mavericklou closed 10 years ago

mavericklou commented 11 years ago

https://github.com/r0man/oauth-clj/blob/master/src/oauth/io.clj#L88

It is really arbitrary to hyphenize the body before output. On client side the return value is all hyphenize after upgrade from 0.0.5. Is there a strong reason to do this?

mavericklou commented 11 years ago

And when body contains nested hash, how is the return value? user=> aaa {:body {:q3 {:data [{:a2 "a2", :a1 "a1"}]}}}

pbostrom commented 11 years ago

The hyphenize does this make this library difficult to use. I will use @mavericklou fork on clojars for now.

canweriotnow commented 10 years ago

What is the hyphenize problem? I find hyphenized (as opposed to underscored, etc.) keys to be more idiomatic Clojure (i.e., principle of least surprise).

r0man commented 10 years ago

me too, but it could be made optional

On Fri, Dec 6, 2013 at 10:59 AM, Jason Lewis notifications@github.comwrote:

What is the hyphenize problem? I find hyphenized (as opposed to underscored, etc.) keys to be more idiomatic Clojure (i.e., principle of least surprise).

— Reply to this email directly or view it on GitHubhttps://github.com/r0man/oauth-clj/issues/9#issuecomment-29976059 .

canweriotnow commented 10 years ago

Maybe optional arg to skip hyphenization? I'm not sure I care enough to submit a PR, but if it doesn't change the API for existing use cases, I don't see it as problematic.

r0man commented 10 years ago

Yes, optional arg to disable. It's also low priority for me.

On Fri, Dec 6, 2013 at 11:27 AM, Jason Lewis notifications@github.comwrote:

Maybe optional arg to skip hyphenization? I'm not sure I care enough to submit a PR, but if it doesn't change the API for existing use cases, I don't see it as problematic.

— Reply to this email directly or view it on GitHubhttps://github.com/r0man/oauth-clj/issues/9#issuecomment-29978131 .

pbostrom commented 10 years ago

I will describe the original issue. In version 1.4 a call to https://api.twitter.com/1.1/statuses/mentions_timeline.json would return output like this: "[{:favorite-count-0,-:entities-{:hashtags-[],-:symbols-[],-:urls-[],-:user-mentions-[{:screen-name- ..." which is pretty much unusable. It is a string which it can't be read by a reader, because all spaces are replaced with hyphens. With @mavericklou commit here: https://github.com/mavericklou/oauth-clj/commit/611ebe204bfb507987e8ad9c8cb6fc2ad10ee9ca The same API call returns: [{:favorite_count 0, :entities {:hashtags [], :symbols [], :urls [], :user_mentions [{:screen_name ... Which is already a Clojure data structure. Looking over the commit history, it looks like the original issue was resolved for 1.6 with this commit: https://github.com/r0man/oauth-clj/commit/11f2a01d538e05f2bf6293e2a6fb6c18a286c26f which now calls hyphenize-keys instead of hyphenize on the body.

I tend to agree that the hyphenized keys are more idiomatic, but it's not a huge deal to me either way.

mavericklou commented 10 years ago

I have to admit that hyphenized keys are something like a standard now in clojure world. Yes, it is more idiomatic. But hyphenize all keys as a default behavior and not well documented are too aggressive.

2013/12/7 pbostrom notifications@github.com

I will describe the original issue. In version 1.4 a call to https://api.twitter.com/1.1/statuses/mentions_timeline.json would return output like this: "[{:favorite-count-0,-:entities-{:hashtags-[],-:symbols-[],-:urls-[],-:user-mentions-[{:screen-name- ..." which is pretty much unusable. It is a string which it can't be read by a reader, because all spaces are replaced with hyphens. With @mavericklouhttps://github.com/maverickloucommit here: mavericklou@611ebe2https://github.com/mavericklou/oauth-clj/commit/611ebe204bfb507987e8ad9c8cb6fc2ad10ee9ca The same API call returns: [{:favorite_count 0, :entities {:hashtags [], :symbols [], :urls [], :user_mentions [{:screen_name ... Which is already a Clojure data structure. Looking over the commit history, it looks like the original issue was resolved for 1.6 with this commit: 11f2a01https://github.com/r0man/oauth-clj/commit/11f2a01d538e05f2bf6293e2a6fb6c18a286c26f which now calls hyphenize-keys instead of hyphenize on the body.

I tend to agree that the hyphenized keys are more idiomatic, but it's not a huge deal to me either way.

— Reply to this email directly or view it on GitHubhttps://github.com/r0man/oauth-clj/issues/9#issuecomment-30007621 .

Factual Confidential Information Any non-public information in this email is Factual's confidential information. It is only for use by the intended recipient under the terms of a non-disclosure agreement or other binding confidentiality terms. If you've gotten this email in error, please let the sender know of the mistake and delete the email immediately.

canweriotnow commented 10 years ago

Okay, I see now... looks like it's hyphenizing the entire body, not just converting the keys...

r0man commented 10 years ago

Ok, I extracted the hyphenize functionality into a middleware which does nothing if the request contains :skip-hyphenize.

(client {:method :get
         :skip-hyphenize true
         :url "https://api.twitter.com/1.1/account/verify_credentials.json"})

Or wrap your own middleware around the client that sets it always to skip mode. Is everyone fine with this?

canweriotnow commented 10 years ago

:+1:

r0man commented 10 years ago

Closing this now