Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Switch string "start" and "rows" to symbols #31

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants

rake spec:api passed locally (using jruby)

Contributor

mwmitchell commented Nov 21, 2011

Hey thanks Erik. This reminds me of something I've thought about a few times... the way rsolr uses keys. I've been tempted many times to bring in something like ActiveSupport's #symbolize_keys method: http://apidock.com/rails/Hash/symbolize_keys but have decided not to because of the dependency. Adding a single method to handle this though wouldn't be so bad.

We could normalize all incoming keys for consistency. This would also be backward compatible so that folks using either strings/symbols wouldn't know the difference.

I'm afraid that this change will break backward compatibility for anyone that's using "start"/"rows", although I'd have to think about this more. I'm not sure why I decided to check for a string key there instead of a symbol though.

What do you think about normalizing all incoming param keys to symbols?

Hmmm.... well, want my personal take on it? A Ruby library, contrary to what I did with solr-ruby, shouldn't do anything with Solr params other than to_s them and encode the values into a URL or POST params. The per_page/page thing shouldn't be there, IMO.

As for the current "rows"/"start" thing... it's inconsistent as-is, since some things look for symbols and then the string stuff was added. Blacklight uses all symbols, so the string thing caused it to double up rows causing an array on the echoParams processing that BL uses. In this particular situation, it could look for either a string or a symbol to ensure there's no doubling up, but I'd rather see everything just be symbols everywhere.

Contributor

Mange commented Nov 22, 2011

Strings are more fitting than symbols, IMO. Solr has an unlimited amount of unique parameter names since the parameter names is almost like a small, small language in itself. I would think converting "f.title.hl.fragsize" and "f.user_id.facet.mincount" to symbols would be a grave mistake.

Symbols should be kept for "special" values. :page would be a case for it since it's not a real parameter, but converted to "start". Now, having it consistent would be better than to have strings for some keys and symbols for some other, but if there's any case for mixing them, this is it.

I agree with @erikhatcher that the keys should just be #to_s by RSolr and nothing else.

Mange - fair enough... it is indeed more awkward to use :'f.category.facet.mincount' than just 'f.category.facet.mincount'.

So I guess the underlying deal here is that RSolr 1) shouldn't care what kinds of param keys are used, but should normalize them internally (wouldn't matter whether string or symbol internally).

Regarding special values - I don't think RSolr should have any special values that get translated into actual Solr parameters. That capability should be relegated to higher levels. There should be total pass-through transparency at the pure RSolr layer for param keys.

Contributor

Mange commented Nov 22, 2011

[...] but should normalize them internally (wouldn't matter whether string or symbol internally).

Actually, I'm also arguing that the internal representation should be strings. Symbol just isn't a good fit for this data.
It's never about the symbol (integer-based, atomic representations) -- it's always about the the string "behind the symbol".

We agree on the rest. :-)

Mange - I don't disagree... it's just irrelevant to the outside what is used, which is the perspective I'm taking in this discussion. Internally, only the RSolr library will see it. So... +1 on normalizing incoming keys, per Matt's initial reply. :)

Contributor

mwmitchell commented Nov 26, 2011

I remember now that the reasoning behind the string keys is exactly what we're talking about here. Native solr params are strings, the :page and :per_page are the exception, which simply get transformed into "start" and "rows".

I can see why you wouldn't want this (pagination) in the library Erik. I've chatted with folks about this before the general consensus was to keep it in. I don't mind so much since it's all pretty simple ... just some math. If you have ideas for alternative implementations etc., I'd love to hear.

@mwmitchell mwmitchell closed this Mar 30, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment