Nice you want to take this on @RhnSharma ! I should've left some more notes on the issue, @danpat is right: the important point of the issue was actually to alias avoid_polygons with exclude_polygons.

It's usually ok to rename things in the config JSON, of course it's also ok to rename the variables. Where it gets tricky:

• we'll need to make sure that the API request can still be successful with the old name as well as the new one. We did smth similar when we deprecated auto_shorter here: https://github.com/valhalla/valhalla/pull/2555/files#diff-dda1eacb6be34d7946468f09844f48890c1266509f634990ec27263271bd6f48
• I'm not exactly sure if it's ok to rename the proto objects.. valhalla is also used as a library and that would be a breaking change for that use case. I'd rather not rename those..

@RhnSharma @nilsnolde yes it is ok to rename the proto objects the protobuffers dont care about the names they just change the name in the generated code, the binary format stays the same so its not a breakign change in protobuf.

As @danpat and @nilsnolde pointed out you need to allow the code in src/worker.cc to parse either avoid_polygons or exclude_polygons. this way existing code that calls this API will still work.

@kevinkreiser Thanks for suggesting the changes. I made the changes, please take a look at it and let me know if you need any changes.

they just change the name in the generated code

@kevinkreiser that's sorta what I mean though: if someone wrote code using valhalla's public PBF interface to create avoid locations/polygons, changing the names would break their code right? Not a huge deal but a bigger deal than breaking the config JSON IMHO

@nilsnolde changing field names in protobuf is considered "binary compatible" - as long as the IDs remained the same and the meaning of the fields is unchanged, then client code generated with old protobuf interfaces will still be able to generate/parse protobuf binary objects successfully without anything breaking.

However these changes are not source compatible - clients calling old generated interface functions will not compile against interfaces generated with these new protobuf changes. It is not well defined what Valhalla considers its compatibility interface to be, but it is not unusual to exclude source-level compatibility for these kinds of reasons.

@nilsnolde changing field names in protobuf is considered "binary compatible" - as long as the IDs remained the same and the meaning of the fields is unchanged, then client code generated with old protobuf interfaces will still be able to generate/parse protobuf binary objects successfully without anything breaking.

@danpat ah didn't know that about protobuf, thanks! that's super considerate behaivor :)

im not sure if we've documented this but this is the general conventions we follow are:

major version change: data incompatibilty, changes to the tiles that makes them not work with previous version soft the software
minor version change: this one is very poorly defined.. sometimes we've done this when we had either larger data changes (eg adding live traffic tiles) or when we add new top level APIs (eg isochrone)
patch version change: pretty much every thing else

We've never maintained source compatibility between any versions of the software as its just impractical and would require way way way too much planning and foresight to be feasible.

@RhnSharma there hsould be some docs changes, specifically you should rename avoid_locations and avoid_polygons here: https://github.com/valhalla/valhalla/blob/master/docs/api/turn-by-turn/api-reference.md#other-request-options to encourage people to use the new more accurate names

also i was thinking about this a bit more and beginning to wonder if maybe it should stay avoid_ to allow us to add a penalty factor down the road. So that a person can soft avoid edges all the way up to hard avoidance. for that we'd add a new parameter to go with the avoids, the locations can easily be updated for that but the polygons might be harder. anyway renaming in this way doesnt technically preclude it but it would make the api more confusing if we ever did decide to do soft user provided avoids. just a thought to ask ourselves here

maybe it should stay avoid_ to allow us to add a penalty factor down the road

There are use-cases for both strict "exclude" (e.g. hazmat, and you want a Noroute response if constraints can't be met) and soft "avoid" (avoid tolls if possible, but take them if costing says it's the best), so ultimately, I think we want both (and this applies to all features of this nature, including use_highways and use_tolls).

My vote here would be to do what @nilsnolde originally suggested - add exclude_ as an alias for the same feature so no APIs break, then later on, fix the avoid_ implementation so that it's a soft avoidance, rather than the current strict one.

@danpat i favored the alias too (it was my suggestion) but i just wanted to throw out the question here and talk myself in a circle one more time ;)

anyway this pr is looking close, whats missing i believe is the other change to the costing options proto didnt get reflected in costing where the avoid edges are used. @RhnSharma you probably need to make some renames in the DynamicCost.h/cc

Makes me think: what do you think about a warnings string array in the response generally? Warnings could be like deprecated things, e.g. using avoid_polygons instead of exclude_polygons or maybe smth like private roads considered when destination roads have to be enabled in a second pass. Anything that's not worth throwing an error but may be worth notifying the user about. Pelias has that since a few years and I find it pretty handy for an HTTP API.

@nilsnolde yeah ive thought about it too. its nice also for those errors in the request that we smooth over without aborting the request too. eg if you set radius on a location above the max allowed we clip it and continue with the request. it would be good to tell the user we arent perfectly adhering to their request

@kevinkreiser @nilsnolde I made the changes you suggested. Please take a look at it and let me know if you need any changes.
Thanks

@RhnSharma are you still planning to finish or should I carry this over the review line? In any case, thanks for the effort already put in!

I quickly finished this. thanks a lot @RhnSharma , was just tiny stuff left

btw @danduk82 &@ismailsunni, this should be relevant for you too.

Thanks @nilsnolde