Factual / factual-clojure-driver

Officially supported Clojure driver for Factual's API.
Other
20 stars 6 forks source link

api's json objects contain invalid keywords #5

Open tomjack opened 11 years ago

tomjack commented 11 years ago

E.g.:

user> (f/facets :restaurants-us {:select "cuisine"
                                 :filters {"locality" "New York"
                                           "region" "NY"}
                                 :limit 50})
{:cuisine
 {;; ...
  :pizza, sandwiches, subs 29,
  ;; ...
  }}

Note that the keywords contain spaces and commas. This looks weird, isn't readable (by the reader, I mean), and is somewhat difficult to work with. I think Strings make more sense here.

It seems clj-http can be made to return strings using :as :json-string-keys, but that causes an error here. Not sure why. Maybe oauth-clj is getting in the way? Of course the tradeoff would be that you get strings even in places where keywords would be appropriate. Hmm...

tomjack commented 11 years ago

I realized it's unlikely oauth-clj is getting in the way — the problem is that keywords are expected in some places in the rest of the code.

Selective keywordization seems doable. We could implement a custom coercion method for clj-http. Of course this would break things for anyone who depended on keywords.

ztellman commented 11 years ago

Hey, sorry for the slow response on this. I think that we should strive for consistency w.r.t. strings vs keywords, and since keywords obviously don't work everywhere, we'll have to settle on strings. I'll be cutting a new version with the requisite changes in the next few days, let me know if you have any questions/comments.

tomjack commented 11 years ago

Thanks, your response was quicker than expected.

I can understand the desire for consistency. Just in case, though, I want to note that by "selective keywordization" I did not mean "keywordize when the keywords would be valid, with a runtime check" but "keywordize when the json object's keys are semantically keywords (in which case, hopefully, they are always valid keywords)".

I'd rather have keywords where they make sense and strings elsewhere than strings everywhere. Of course this would require some thought (or inspection of results) by the user, but I don't see it causing too much trouble. I suspect the places where invalid keywords might be returned are exactly the places where a user probably expects strings.

ztellman commented 11 years ago

Could you give me an example of where keywords would be appropriate, in your opinion? Just going on the principle of least surprise, I agree not using keywords kind of sucks, but unless there's a simple heuristic for where they're used and where they're not, the mixed approach could easily be worse.

tomjack commented 11 years ago

some places I expect keywords:

In general, I expect keywords when the key represents a schema field name or serves as structure for a response. Anytime the key is a piece of place data (whether or not all the values in that column happen to be valid keywords), I expect strings.

The only places I've noticed (so far..) where I expect strings are:

A heuristic that occurs to me is: if you could, theoretically, replace every instance of the key in all of Factual's data and everyone's code with a gensym, without losing anything except readability by devs, it's a keyword (assume people don't call name on these keywords...). Otherwise, it's a string. Keywords are, semantically, opaque labels, while strings are, well, strings.

I do understand though if you decide it's simpler to just use strings everywhere.

dirtyvagabond commented 11 years ago

@ztellman i think we let this fall through the cracks. mind picking it up after your Clojure West adventures?

dcj commented 11 years ago

Just returning to this after a long time of non-use (busy with other things), previously I had used the "funny places" version....

I would like to strongly second the comments from @tomjack As Tom stated: "In general, I expect keywords when the key represents a schema field name or serves as structure for a response" I definitely agree with this.

Actually, I do not yet understand why any key that you return would need to be a string. Everything returned by:

(map #(get % "name") (fact/schema :restaurants-us))

could easily be a keyword, and should be!

I have zero/no experience with the facet feature, but after a very cursory look at this, my POV is that it is kinda unimpressive if the way multiple items are specified is by stringifying them with comma separators. Really? How about specifying a facet like this:

:select [:locality :region]

Same thing on the output side, I think. I confess I do not yet understand the query and the expected response of the example given above:

  :pizza, sandwiches, subs 29,

But if the intent is to state that there are 29 restaurants in the locality that serve "pizza, sandwiches, and subs", where each of those is a recognized Factual cuisine/category, then I would propose the map returned look like:

{:cuisine
  {;; ...
   [:pizza :sandwiches :subs] 29,
   ;; ...
  }
}

If a user wants a prettier string/label/name than the keyword, then presumably there are ways to go from the keyword (i.e the keywordized "name") to the nice string which is/should-be the value of "label" in your schema.

In summary, I have a stronger opinion than Tom, offhand I cannot think of any place where string keys would be appropriate in a request, and the only place I can imagine they might be appropriate in a response is when the keys in the response are from some "real word" data which is not part of your schema.....