Hi,
I just merged the current GH master into my codebase and started to get a massive headache ;).
I had several “deprecated” hasErrors checks in my code and I always ended up with java.lang.IllegalStateException: No alternative existent. I saw that it is probably intended to use the hasRawErrors() method.
So we should at least add this to the changelog. Or maybe think about if having no alternative is actually an Error in this case (in general it most probably is).
Cheers,
Robin
How about adding a method called isValid which checks if there is an alternative or an Error and revert the hasErrors function to what it was?
Somehow I feel like having a response without an alternative is actually not an Error, but is not valid. This would at least help me to read the code more easily.
Yes, this is not polished. Will think about this and delay the release until this is better.
I stumbled over this problem too while refactoring and it is indeed ugly that the meaning of hasErrors changed somehow.
The underlying problem why I still decided towards this was that we need the ‘alternative error’ in the error list - or do we really need it there? And !getErrors().isEmpty should behave identical to hasError.
Would you please describe in your case again which methods you called and why the error No alternative existent wasn’t appropriate?
E.g. for GraphHopperWeb I needed to change the code to:
res.addErrors(readErrors(json));
if (res.hasRawErrors())
return res;
as otherwise it would always return as no alternatives were parsed from the JSON but I thought that this usage was a bit special…
So we should at least add this to the changelog. Or maybe think about if having no alternative is actually an Error in this case (in general it most probably is).
Yeah, this is the least we should do, but let us think about this again.
I had the same thoughts too, trying to understand how to integrate the new error handling in my code.
For example, playing with GH without alternative routes enabled and putting one point in the sea, besides the usual “Cannot find point…”, I receive also “No alternative existent” though I didn’t asked for one.
Hmmh, indeed and ugly again. Then maybe we should not handle none existent alternatives as explicit errors.
But what to do if no alternative is existent? As then getFirst etc would throw an uncaught exception instead of being added to the error list.
Shouldn’t the alternatives error only be produced when we have asked for alternatives in the first place and not found?
Regular routing with only 1 expected route could stay with the usual error messages.
My use-case was actually quite similar to the calcPaths method. I was processing something in a method and after the method I queried the response for hasErrors. But at this point no paths were calculated.
Every route is an alternate route right now. So if there is only one then this is an alternate route.
I think alternate usually means an entity side by side the original.
So that message kind of tells it couldn’t found a 2nd route, not the 1st one.
That’s what users used to understand so far - if not aware of the new API.
At the end it all comes down to how we want to express the meanings.
Indeed maybe the naming is confusing too. So at the moment in JSON we just have multiple entries in the paths array (which makes sense IMO) and in GHResponse we have multiple AltResponse where the first one is the best path (which makes only partly sense). If we would separate the ‘best’ response and the ‘alternative’ responses we have a problem with iterating over all. Hmmh.
Would the following solve the issues?
Rename:
AltResponse -> PathResponse
GHResponse.getFirst -> getBest
GHResponse.alternatives -> paths
GHResponse.getAlternatives ->getPaths
And then introduce hasAlternatives to see if paths.size > 1 and getErrors and hasErrors back to normal, furthermore we assume that if there is no path at all we always expect at least one explaining error in GHResponse.
Yes that seems a more understandable description of the various elements.
This a list of responses (not of actual Path class), so how about pathResponses and getPathResponses ?
Not sure, as when it would be used it would be a bit duplicated like response.getPathResponses. Maybe a different name should be used? Like PathWrapper or PathData?
As the data inside the current AltResponse is what is necessary from a Path list (Paths merged via PathMerger) forming the route we could also name it ViaPath or Route. Still something with path would be preferable for now as we have paths in JSON too for them.
Yes this would improve the current situation. Maybe we could also call it extractedPath or potentialPath. I am not sure about the naming either.
hmmh, and then ghResponse.getPaths or better ghResponse.getPathWrappers 
? I tend to pick the first …
I’ve done a proposal here: https://github.com/graphhopper/graphhopper/commit/ebf717c3ff0319fc3513b0213c2706a04faca863
And WDYT about GHResponse.getAll instead of getPathWrappers and still GHResponse.pathWrappers would be used.
It’s seems ok, probably at the description of GHResponse.hasErrors the “if no alternative is available” is not needed now?
This is way better now. Thanks 