Add detailed information about Function arguments and return type to FunctionName

Jody Garnett added a comment - 31/May/11 9:51 PM Okay Ben and myself stalled out trying to hit the implementation side of things with respect to validation: - factory creation of function name should not be subject to any restrictions we have on the GeoTools side of things (as we need the data structure to communicate the capabilities of external services and sql functions etc...) - existing symbology encoding functions support several parameters with multiplicity (not just the last one) - existing app schema functions support several optional parameters (not just the last one) With that in mind I would like to remove the validation check; if a function implementor wants to take on the pain of sorting out things they should be able to do so. Note this does not require the introduction of named parameters or any change to the FunctionFinder (lookup is on function name only). Options: a) just remove the validation check b) remove the validation check from the factory and the direct FunctionNameImpl calls; leaving it in the utility method as a way to encourage function implementors from taking on too much work

Justin Deoliveira added a comment - 01/Jun/11 9:21 AM Ok, how about this patch. Basically removed the validate() call from the FUnctionNameImpl constructor and move it to FunctionImpl.dispatchArguments(), which is a new method that functions can call to dispatch arguments according to the java convention for variable arguments. No functions currently call that method (except for one test function) so it shouldn't break anything. So basically functions are responsible on their own for handling function arguments, unless they call the method in which they are forced to follow the conventions.

Jody Garnett added a comment - 01/Jun/11 7:25 PM 1) the patch looks good and should allow us to proceed (thanks!) 2) The validate() method could describe that it throws IllegalArgumentException in the javadocs (or even explicitly with a throws IllegalArgumentException (I had to read the code to sort out why it was not returning "true or false". I agree that throwing an exception is nicer as it gives the caller an actual error message. private void validate() throws IllegalArgumentException {    ... } 3) Now understand your comment about dispatch (from the proposal we did not know about prepArguments producing a Map<String,Object>. I think function implementors will like this one.      Reading the code I wonder why you call evaualte( feature ) - this amounts to evaulate(feature, Object) and does not allow the functions to perform their own coversion as per the API contract. This is functionality I have dependend on in the use in order to allow a string to be converted to an enumerated type for example).      So I would update the code to have:          Object o = expr.get(i).evaluate(obj, parameter.getType() ); I understand that Andrea is tired of the filter / expression system being forgiving and would like to see real error messages. The conversation deserves its own Jira; but this change does finally enable us to provide real error messages.          Object o = expr.get(i).evaualte( obj, parameter.getType() );     if( o == null ){           // check if it was a conversion error           o = expr.evaulate( obj );           if( o != null ){                  throw new IllegalArgumentException( "Function "+name+" requires argument "+parameter.getName()+" to be "+parameter.getType()+". Unable to use:"+expr+" with value: "+o"");           }     }

Justin Deoliveira added a comment - 02/Jun/11 9:51 AM Thanks for the review Jody. This patch should address all the feedback.

Andrea Aime added a comment - 02/Jun/11 12:24 PM Seems all good to me too

Justin Deoliveira added a comment - 03/Jun/11 11:55 AM Patch committed.

Jody Garnett added a comment - 04/Jun/11 9:16 AM I would like to keep this open until we are done the work for the proposal (no half measures etc..). I have had some good luck using the following approach: import static org.geotools.filter.capability.FunctionNameImpl.parameter; ...     public static FunctionName NAME = new FunctionNameImpl("rint",             parameter("related", Boolean.class),             parameter("geometry", Geometry.class),             parameter("geometry", Geometry.class),             parameter("pattern", String.class));

Andrea Aime added a comment - 03/Jul/12 3:13 AM Aren't we done already?

Jody Garnett added a comment - 03/Jul/12 4:21 AM I think we may be. I have slotted http://docs.codehaus.org/display/GEOTOOLS/Detailed+Argument+and+Return+Info+for+FunctionName in under 8.x.