Closed nreese closed 7 years ago
Thanks for the contribution. Can you update documentation showing basic usage? Maybe just a table showing the supported grid strategies (basic, metric) with description.
Supporting throwing exceptions in computeCellValue makes sense to me as well. I don't have any preference on type of exception (new custom exception, IllegalArgumentException, etc.) but whatever it is it should probably be caught and wrapped in a ProcessException in GeoHashGridProcess. If you get in there feel free to remove the iae.printStackTrace();
... just noticed that and not sure how it snuck in.
I also think supporting an argument for the key makes sense. To generalize the capability I'd recommend adding support for a new parameter, computeArgs, in GeoHashGridProcess. Hopefully the type could be String[]
or List<String>
. The user would provide these args through another sld function parameter and they'd then be passed on to the computeCellValue implementation. Docs could be updated describing what (if any) computeArgs are required for the given strategy. What do you think?
I am thinking about a new custom exception. That way, we can log a very friendly, easy to understand message about why the GeoHashGridProcess.execute method returned null. I am thinking this will be a very common mistake since the values are defined in two places (SLD and WMS parameter) and it will be super easy to have a mismatch. What about calling it a KeyNotFound exception?
Can SLD parameters support maps? It would be nice to have key value pairs passed to the GeoHashGrid.
Sounds good to me. Regarding parameters, I've only ever done parameters with basic types or feature collections. The docs say lists/arrays are supported but I'm not sure about maps. If maps aren't supported I think an array wouldn't be too bad ... docs would just have to describe what the first, second, etc. value represents for the relevant strategy.
I decided to just throw an IllegalArgumentException
from inside computeCellValue
when the bucket does not contain the required keys. I tried the custom exception route but lambda functions do not play nice with checked exceptions and things were getting messy.
This looks good to me. Are you done for now on this or are you still looking at adding support for the additional array/map computeArgs
parameter? I think there's value here even without the parameter but I'll wait on merge if you're working on that.
Lets go ahead and merge. I am working on another GeoHashGrid implementation for dealing with nested buckets. I will figure out the parameter stuff on that one.
New GeoHashGrid implementation that allows for pulling metric values from the geohashgrid bucket.
Should there be better error handling? What if the bucket does not contain the key
metric
? How should this method respond? I was thinking that maybecomputeCellValue
method should throw an exception in these situations. How does that sound? What should the Exception type be?Is there any way to pass parameters to GeoHasGrid from the SLD? Maybe the key should not be hard coded but passed via a parameter. Thoughts?
Usage example with the
max
metric