Replies: 2 comments
-
I'm not (yet) using valhalla as a library, but I totally agree, esp about config changes (other sem ver stuff is mostly an oversight I think, like the logger issue). with the config we're being very brutal for consumers. there have been numerous issues about this, e.g. #2938, but no clear path forward AFAICT. I do want to start using valhalla as a library for QGIS soon, then I'll get hit by the same thing.. there's numerous other consuming frameworks which should also be interested in seeing this improve. myself, I'm not too experienced with c++ and not sure what the best way would be. I am willing to help solve it though. I think a good start would already be to pay more attention to not make new keys required and have sensible defaults (there's very little really required keys that can't have defaults, like the tile paths). I think the PR template could be extended accordingly, so both reviewer and PR person take more care. I'll let others light the way ahead how this could be solved long-term. |
Beta Was this translation helpful? Give feedback.
-
Yeah so our take on semver has to do more with backwards/forwards compatibility of the data. Here's a brief outline:
We have been very good about sticking to major revision changes. I will say that we don't do such a good job about making a minor version change when new changes to the data take place. For example we could have upped the minor version when we added phonetics, though in that case we were still just making use of an existing data storage concept within EdgeInfo, so its a bit of a grey area. As for patch revisions since thats everything else we havent really transgressed there. I hear what you are saying though. We dont, at least this time, use semver to indicate anything in the way of c++ API changes. I think its perfectly reasonable for you to expect that you can use libvalhalla in your downstream application, no problem there. But I wil say that on a practical note I dont think we will be getting to a place where we police the c++ API any time soon. I think it would be possible if we would make the effort to take all the existing public apis (stuff in public headers) and slim them down as much as possible moving as much implementation out of the headers as possible and into their respective compilation units. This is a huge task, which is why i dont think we'll be getting there any time soon, but it is possible. Regarding the specific config issue, it is well known, as nils pointed out above, i replied in at least that one issue about how to go about doing this (porting the config to a class inside c++ with all the defaults living there). I think that that is pretty well surmountable since we've now got a model in which everythign is pretty much optional or has a reasonable default: #2938 (comment) If the m ain thing holding you up is the config I suggest we do something like is done for the tests. Take a default json, wrap that in a config class and add some convenience methods to it. We'd need to then create a valhalla_build_config binary to replace the python version too. I think that would be the minimum we could get by with to at least work around the specific issue of not being able to generate config from the c++ API. |
Beta Was this translation helpful? Give feedback.
-
I have been using Valhalla as a C++ library as a backend for routing and related services of OSM Scout Server. I presume it is not a typical way of using it as I constantly stumble upon unexpected changes in API and configuration. For example, update from 3.1.1 to 3.1.4 led to breakage due to:
valhalla::midgard::logging::LogLevel::LogError
instead ofvalhalla::midgard::logging::LogLevel::ERROR
These changes were introduced what is expected to be "patch" level bump in the version according to Semantic Versioning. I guess we can conclude that Valhalla doesn't follow semantic versioning, but maybe it could do so to.
Valhalla is a great piece of software and it is a privilege to be able to use it on mobile and desktop Linux distributions. Tile format reader has been backwards compatible for a while and breakage in the format was introduced on version 3, if I remember correctly. Well in accordance to the bump in the major version. Maybe the other aspects of versioning could be followed as well.
Changes in JSON config settings as appearance and disappearance of config keys is a major frustration when working with the library. They always lead to some kind of exception on reading the config. I haven't found a way of getting
boost::property_tree::ptree
filled with the default options for Valhalla by Valhalla itself. It seems that the default config has to be generated by Python script. If C++ API with default config would be available, I would be able to load it and change in accordance with the user preferences (location of tiles, limits for routing and other calculations).Please consider it as mainly a feedback on small issues that I get while using Valhalla as a library. As you could see, I am opening these as a "discussion" and not an issue as I am not sure it warrants issue status as such.
Beta Was this translation helpful? Give feedback.
All reactions