Filter manipulation Vistors and utilities contribution

Eric Sword added a comment - 13/Apr/11 8:49 AM Re Comment #3: Sorry about the missing public declaration. That's a remnant from Groovy where methods default to public rather than package. My unit tests didn't catch it because, of course, the unit test is in the same package. As for whether to keep the FF or not, I found it useful to have a standard, pre-built one with good defaults in the apps I have used the utils class in, especially for other developers who needed to use one but who didn't know the details of geotools or creating one. However, if these methods are merged into the Filters class and we drop the "addFilter" method in favor of the existing "and" and "or" methods, then we can sidestep the issue because none of the other methods need a factory. Other responses coming. Just have to get through an interminable conference call first.

Jody Garnett added a comment - 13/Apr/11 9:42 AM Hey Eric nice of you to show up; I am just about to commit the results of this first bash of reviews. I was able to take on everything except your PrePostFilterVisitors; need some more thought there. If you could adapt your groovy test cases to abuse the result we should be able to find a few corner cases to kick around. I am a believer in lazy test cases; where each problem gets a test case so we can verify when it is fixed. I do have at least one test case on each Filters method now so we can notice if we break something.

Eric Sword added a comment - 13/Apr/11 10:07 AM Re Comment #1: AbstractSearchFilterVisitor makes sense because the two Filters where the capability is most useful are the two I originally wrote - FindType and FindProperty. Those do want to check every type of Filter that comes through. The only reason for the instanceof check in FindProperty is because the parent classes implement ExpressionVisitor as well. As for the other two (GetParent and Replace)... mea culpa. I have to think back to when I wrote them a bit over a year ago and guess that I just got in the habit of extending PrePostFilterVisitor rather than thinking about what fit best. Having them extend DefaultFilterVisitor and then override And/Or/Not is cleaner. Or, generalizing the use case (which I tend to do too much), we could have "grouping visits" that are called for certain types of related items. e.g. preVisit/postVisit[All, Filter, Expression, Logic, BinaryComparison, BinarySpatial, etc]. With these, FindType would override postVisitAll, FindPropertyVistor would override postVisitFilter and GetParent and Replace would override postVisitLogic. (Pardon the names.) I am just tossing around ideas to avoid having many one line functions that call into some other function to do the guts of their work as will happen with GetParent and Replace when overriding the visit methods for And/Or and maybe Not. This isn't an urgent problem in any sense since only these classes do it at the moment. It is probably easier to just wait and see if additional classes could benefit from having a set of "functional grouping" visit calls.

Jody Garnett added a comment - 13/Apr/11 6:52 PM You are onto something with how you use your PrepPostFilterVisitors; I am just not sure what :-) One common thing is that you are trying to look around (parent and child relationships) as you process a filter ... 1- we already have one good way to "look down" at the children (pass control to another visitor which will go down and have a look) 2- but there is no good way to look up at parent So #2 is our real trouble here; I have an idea ... when we are writing the duplicate visitor we have to keep a stack around so we don't go insane with recursion. If we make a visitor that keeps a stack around then in your normal visitor methods you could ask the stack questions about your parents. Would that work for you?