SIP78 Convert switches to run time options#96
Conversation
dlebauer
left a comment
dlebauer
left a comment
There was a problem hiding this comment.
Looks good to me, given my limited experience.
What are your plans regarding testing this core new functionality? I don't want to require it here, just want to make sure it is on the radar (e.g. after all the major refactoring is complete?)
| printf("\n"); | ||
| printf("Info options:\n"); | ||
| printf(" -h, --help Print this message and exit\n"); | ||
| printf(" -v, --version Print version information and exit\n"); |
There was a problem hiding this comment.
exciting news!
$ ./sipnet -v
SIPNET version 2.0.0
| } | ||
|
|
||
| // Print the version when requested | ||
| void version(void) { printf("SIPNET version 2.0.0\n"); } |
There was a problem hiding this comment.
Thanks for adding this! Though I don't think we want to hard code this ... could this go in VERSION or src/version.h?
There was a problem hiding this comment.
It definitely should go somewhere not here - I was thinking src/version.h too, but was leaving it open for now (obviously). I'l switch to that, and we can see how we feel about that.
| printf("Flag options: (prepend flag with 'no_' to force off, eg '--no_print_header')\n"); | ||
| printf("\n"); | ||
| printf(" --dump_config Print final config to <input_file>.config (0)\n"); | ||
| printf(" --print_header Whether to print header row in output files (1)\n"); |
There was a problem hiding this comment.
Is the default really on?
./sipnet --input_file tests/smoke/sipnet.in
head -n 2 tests/smoke/niwot.out
0 1998 305 0.00 5759.77 1133.88 16000.06 8.00 1919.90 1919.64 400.00 0.500 6.00 0.500 0.00 -0.32 0.74 0.74 0.00 0.164 0.578 0.159 0.324 0.419 0.742 0.00302126 0.0000 0.0000
0 1998 305 7.00 5759.63 1133.71 16000.08 8.00 1919.77 1919.10 400.00 0.500 5.99 0.500 0.00 -0.30 0.97 1.71 0.22 0.271 0.917 0.251 0.522 0.666 1.188 0.00240544 0.0022 0.5821
There was a problem hiding this comment.
Urm... no, it isn't 😬
Well, to be clear - our coded sipnet.in files explicitly turn it off. The actual default is currently on.
There was a problem hiding this comment.
in fact, I strongly suspect all existing sipnet.in files turn it off, unless someone wanted it on. Or, more to the point, I bet that all existing sipnet.in's set it to something, so the default is somewhat moot.
| (!strcmp(mode, "r") || !strcmp(mode, "rb")) ? "reading" : "writing"; | ||
| fprintf(stderr, | ||
| "Error %s '%s': %s\n", | ||
| mode_word, name, strerror(errno)); |
There was a problem hiding this comment.
I made this msg more helpful, and in the process learned about the strerror function, though I still don't exactly understand how it finds "no such file or directory" from SIPNET's custom error codes!?
before:
Error opening sipnet.in for r
after:
Error reading 'sipnet.in': No such file or directory
Error writing 'tests/smoke/niwot.out': No such file or directory
There was a problem hiding this comment.
I like this better too!
strerror isn't using sipnet's error codes; that errno is a bit of system global variable trickery, it's actually the error code from the fopen call.
| CREATE_CHAR_CONTEXT(inputFile, "INPUT_FILE", DEFAULT_INPUT_FILE, CTX_DEFAULT); | ||
| CREATE_CHAR_CONTEXT(runType, "RUN_TYPE", RUN_TYPE_STANDARD, CTX_DEFAULT); | ||
| CREATE_CHAR_CONTEXT(fileName, "FILENAME", "", CTX_DEFAULT); | ||
| CREATE_INT_CONTEXT(location, "LOCATION", 0, CTX_DEFAULT); |
There was a problem hiding this comment.
will references to location and LOC be obsolete following #92?
| // strip names down by removing chars like dashes and underscores, and | ||
| // converting to lowercase. This will allow different versions to still find | ||
| // the relevant metadata, while retaining uniqueness. | ||
| void nameToKey(const char *name) { |
There was a problem hiding this comment.
should this normalization be documented? i.e. is this a 'feature' or should we expect users to use the correct form but silently accept others?
There was a problem hiding this comment.
Well, that's a good question! This is here partly to reconcile params like RUNTYPE in existing sipnet.in files, but also differences (at least in appearance) between command line options and input file naming.
I'd say, when we get to documenting putting these in the input file (which might be part of the next PR? Maybe a follow-on?), we go ahead and doc it. Hopefully people won't start using In&PuT-*_fiL:E!, for inputFile, even though it would work, lol.
|
Re: testing There should be no change in model functionality from this (and the follow-on) PR; I think a good plan would be to do #103:
|
2 participants
#78
This is Part 1, since this is a good place to partition the work, and I've made enough huge PRs recently...
This PR:
sipnet.inand the command line, merging them into the Context object (command line has precedence)uthash, a third-party header file) to manage metadata about context params, including what source the param value comes from<inputfile>.config(e.g.,niwot.config)namelistinput, as that is superseded by the ContextPart 2 of this task will be to convert the compiler switches that we are keeping as options to run-time args, update the Context and cli for these options, and remove/massage code related to the flags that we are removing or hard-coding on.