Fwd: svn commit: r1001318 - in /lucene/dev/trunk/solr: CHANGES.txt src/java/org/apache/solr/search/FunctionQParser.java src/test/org/apache/solr/search/QueryParsingTest.java src/test/org/apache/solr/search/function/TestFunctionQuery.java

[Grant Ingersoll <gs...@apache.org>]

Yonik,

I'm in the process of backporting, and this commit and the subsequent revert on QueryParsingTest looks weird, so I thought I would double check the intent.  See below

Begin forwarded message:

> From: yonik@apache.org
> Date: September 25, 2010 5:06:11 PM EDT
> To: commits@lucene.apache.org
> Subject: svn commit: r1001318 - in /lucene/dev/trunk/solr: CHANGES.txt src/java/org/apache/solr/search/FunctionQParser.java src/test/org/apache/solr/search/QueryParsingTest.java src/test/org/apache/solr/search/function/TestFunctionQuery.java
> Reply-To: dev@lucene.apache.org
> ==============================================================================
> --- lucene/dev/trunk/solr/src/test/org/apache/solr/search/QueryParsingTest.java (original)
> +++ lucene/dev/trunk/solr/src/test/org/apache/solr/search/QueryParsingTest.java Sat Sep 25 21:06:10 2010
> @@ -133,13 +133,7 @@ public class QueryParsingTest extends So
>     flds = sort.getSort();
>     assertEquals(flds[0].getType(), SortField.FLOAT);
>     assertEquals(flds[0].getField(), "weight");
> -
> -    try {
> -      //bad number of parens, but the function parser can handle an extra close
> -      sort = QueryParsing.parseSort("pow(weight,2)) desc, bday asc", schema);
> -    } catch (SolrException e) {
> -      assertTrue(false);
> -    }
> +gvim
>     //Test literals in functions
>     sort = QueryParsing.parseSort("strdist(foo_s, \"junk\", jw) desc", schema);
>     flds = sort.getSort();

Namely, it looks like like we had a test that checked for dealing with extra parens.  Then, perhaps you made some typo that inserted the chars: gvim in place of that test.  Subsequently in rev 1001320 you kind of reverted the commit by removing the gvim, but did you intend to restore the test or are you seeing it as a bad test?

-Grant

RE: svn commit: r1001318 - in /lucene/dev/trunk/solr: CHANGES.txt src/java/org/apache/solr/search/FunctionQParser.java src/test/org/apache/solr/search/QueryParsingTest.java src/test/org/apache/solr/search/function/TestFunctionQuery.java

[Steven A Rowe <sa...@syr.edu>]

On 12/2/2010 at 1:34 PM, Yonik Seeley wrote:
> the window you see isn't necessarily the window that has the focus :-(

<rant>
A graph of the number of times I've made this mistake over time would look a lot like a flat line.

The fact that this is such an easy mistake to make at this point in UI evolution ought to be a red flag for interface designers.

Users need more/better input-focus cues.

With desktop computers' increasingly larger display real estate and processing power, and hence the increasing number of (visible) open windows, the probability of any given window having input-focus is trending lower, so the likelihood of inadvertent input placement is growing.
</rant>

Steve

Re: svn commit: r1001318 - in /lucene/dev/trunk/solr: CHANGES.txt src/java/org/apache/solr/search/FunctionQParser.java src/test/org/apache/solr/search/QueryParsingTest.java src/test/org/apache/solr/search/function/TestFunctionQuery.java

[Yonik Seeley <yo...@lucidimagination.com>]

On Thu, Dec 2, 2010 at 12:58 PM, Grant Ingersoll <gs...@apache.org> wrote:
[...]
>     flds = sort.getSort();
>     assertEquals(flds[0].getType(), SortField.FLOAT);
>     assertEquals(flds[0].getField(), "weight");
> -
> -    try {
> -      //bad number of parens, but the function parser can handle an extra
> close
> -      sort = QueryParsing.parseSort("pow(weight,2)) desc, bday asc",
> schema);
> -    } catch (SolrException e) {
> -      assertTrue(false);
> -    }
> +gvim
>     //Test literals in functions
>     sort = QueryParsing.parseSort("strdist(foo_s, \"junk\", jw) desc",
> schema);
>     flds = sort.getSort();
>
> Namely, it looks like like we had a test that checked for dealing with extra
> parens.

> Then, perhaps you made some typo that inserted the chars: gvim in
> place of that test.

Yeah - the window you see isn't necessarily the window that has the focus :-(

>  Subsequently in rev 1001320 you kind of reverted the
> commit by removing the gvim, but did you intend to restore the test or are
> you seeing it as a bad test?

It was a bad test.
Tests should test for desired functionality, not quirks or
implementation details of the current implementation.
The fact that the past implementation did not error on something like
"add(a,b))"
was a quirk/bug, not a feature.

-Yonik
http://www.lucidimagination.com

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org