On 6/23/06, Chris Hostetter <[EMAIL PROTECTED]> wrote:
Mike, I've only had a chance to skim your patch so far, but on the
surface it looks very good to me. One little thing scares me a little
bit...
: 5. SolrPluginUtils.getParam() only uses the default parameter if it is null,
not blank.
...in principal, I agree with the sentiment that "" should be a legal
value, but the Utils class was implimented this way to support URLs
generated by HTML form submission, where the param is still included in
the URL but with an empty value. Changing this would break at least one
or two Solr uses I know of.
I thought that this might be problematic which is why I made sure to
highlight it. Definitely an issue that should be resolved as early as
possible Solr's development. Note that there is already at least one
parameter where presense/absence is significant (debugQuery).
Then again, if we want to change it, now would be better then latter
... hmmm. Perhaps an alternate approach might be to continue to use the
default when the param is empty (""), but stop trimming the values before
the comparison, so that " " is a legal value that prevents the default
form being used?
Significant whitespace scares me <g>. If presence/absense of a param
being significant is problematic, it is better to simply disallow this
Solr-wide. Mandate that value-less parameters are equivalent to not
specifying them at all. Things like the highlight parameter can
accept 'true' as a hint to use the default field for these types of
uses, or it can be broken down into two parameters
(highlight=true/false, highlightFields=...).
I'm eager to see what other people on the list (who know more about
highlighting and are interested in using highlighting with Solr) think
about your patch ... alas, Erik seems to be quite busy right now, maybe he
can share some thoughts next week? (fingers crossed)
Sounds good--I'm busy for the next week regardless.
Thanks for your comments!
-Mike
