On Mon, Nov 19, 2012 at 3:23 PM, Eirik Lygre <[email protected]> wrote: > > > On Sat, Nov 17, 2012 at 11:41 AM, Eirik Lygre <[email protected]>wrote: > >> >> // Functional, symbolic api. Changes are serialized immediately >> Map<String, ORDER> getSort() // Returns immmutable, ordered map >> setSort(LinkedHashMap<S,O>); >> addSort(String, ORDER); >> removeSort(); >> removeSort(String); >> >> So, a couple of questions out of this: >> - Do we want to maintain backward compatibility i.e. the use case with >> setting the string and then manipulating it? >> - Is there a clear point for serialization, so that we can serialize "at >> the end" only? >> > > Two comments for the api: > > 1) There is a use case that might be fairly common, in which first a > number of sorts are set, and then the user wants to change the direction of > that sort. Presumably this happens over time, and over several user > interactions, but it would probably look like this: > > q.addSort ("a", ASC).addSort("b", ASC).addSort("c", ASC); > // Now change the sort order of a, while maintaining a as the first field > in the sort list > q.setSort("a", DESC); // Don't like; setSort() feels like it should remove > others > q.toggleSort("a"); // Works, but I'd prefer an api where I decide the > direction (e.g. not toggle) > q.toggleSort("a", DESC) // Works, but the word toggle is not really good > q.changeSort("a", DESC); // Works, though it is not clear what to do if > "a" is not already in the sort list > q.changeOrAddSort("a", DESC); // This is my preferred semantics, but what > about the method name? > q.setSort(new LinkedHashMap(q.getSort()) > > 2) Also, I'm not sure I like the method name "removeSort()" to remove all. > Either "removeAllSorts()" or "clearSort()", with a preference to the latter > (it matches "clear()" which is used to clear the entire > SolrQuery/ModifiableSolrParams) > > // Functional, symbolic api. Changes are serialized immediately > clearSort(); > Map<String, ORDER> getSort() // Returns immutable, ordered map > setSort(LinkedHashMap<S,O>); > addSort(String, ORDER); > changeOrAddSort (String, ORDER) > removeSort(String); >
Attached is a patch; I'll put it up on jira once we're kinda happy with it :-) 1) I renamed the new method (#1 above) to "addOrUpdateSort()". Probably not a particularly good parallel, but I found org.apache.lucene.index.FieldInfos.addOrUpdate(), and reused that convention 2) I removed the new raw string methods getSortString() and setSortString(). The point of this exercise is to use a symbolic api, and if you want the raw field, use get()/set() with CommonParams.SORT. The attached patch now contains a few fixes to the old api, tests for those fixes, the new api with javadoc, and tests for the new api. -- Eirik There is no high like a tango high There is no low like a tango low
SOLR-3926.patch
Description: Binary data
--------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
