move argparse boilerplate code to private header #4169
Conversation
|
Apart from adding the utility function, what it generally does:
|
There was a problem hiding this comment.
ok so this supercedes the previous one but a couple things:
- can we put the interface for the elevation get_tileids back to unordered_set
- can we not use relative paths to include the private headers, looked like cmakelists already had the source dir has an include directory
| options.allow_unrecognised_options(); | ||
| std::string config_file; | ||
| int main(int argc, char** argv) { | ||
| const std::string program = "valhalla_add_elevation"; |
There was a problem hiding this comment.
you can get this by messing around with __FILE__ then you dont have to type every single one though i guess it has to be the name of the source file (which we do adhere to)
There was a problem hiding this comment.
jep, that worked, thanks! made it a function in argparse_utils.h
done
had to adjust the CMakeLists.txt a bit and add the entire |
…, we need to put the ./src directory to target_include_directories()
nilsnolde
force-pushed
the
nn--exe-boilerplate
branch
from
cdd4fd7 to
9d81545
Compare
src/argparse_utils.h
Outdated
| if (basename.length() <= 3) { | ||
| return ""; | ||
| } | ||
| basename.erase(basename.length() - 3); |
There was a problem hiding this comment.
nit: are you sure you dont want to rfind '.', i know its pedantic but its robust to us one day changing from stupid .cc for to the more prevalent .cpp/.hpp
valhalla/baldr/graph_utils.h
Outdated
| @@ -6,14 +6,14 @@ | |||
|
|
|||
There was a problem hiding this comment.
this whole file seems misplaced. could we just move the functions into an existing utils header?
There was a problem hiding this comment.
I didn’t feel it’s fitting anywhere.. def not mjolnir, and it’s not really midgard material.
There was a problem hiding this comment.
why dont we just delete this file entirely and then we can change the interface of elevationbuilder::build to take an unordered_set and make a dequeue out of it and check the filesystem etc if that is what it needs. just move the functionality into the main function, or, if it means testing is less verbose thats ok with me. rather have less random files hanging around tbh
| @@ -228,6 +228,7 @@ TEST(ElevationBuilder, get_tile_ids) { | |||
| // check if config contains tile_dir | |||
| EXPECT_TRUE(valhalla::mjolnir::get_tile_ids(config, {"/0/003/196.gph"}).empty()); | |||
|
|
|||
| // TODO: I don't understand what the following lines have to do with anything? | |||
There was a problem hiding this comment.
above this uses the default config, i suppose if on your local computer you had elevation data in the default location it would pick that up and change the result of this test. to avoid that the person removes this value so that it cant happen
|
this looks done to me, i woul just remove the extra file and push that logic into elevationbuilder::build and call it a day |
| rapidjson::read_json(config.c_str(), pt); | ||
|
|
||
| // configure logging | ||
| valhalla::midgard::logging::Configure({{"type", "std_err"}, {"color", "true"}}); |
There was a problem hiding this comment.
oops we wanted to keep this stderr #4496
Removes a few lines of code by breaking out argparse stuff of the programs into a private header.
The PR is based on #4168