-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixes issue where command line ignored if YAML exists #62
Conversation
… Cleaned up code accordingly and tested. Removed compiled code. Not sure how to add binaries to a .gitignore but I think it would be a good idea to do so.
Signed-off-by: Max von Hippel <maxvonhippel1996@gmail.com>
Signed-off-by: Max von Hippel <maxvonhippel1996@gmail.com>
Signed-off-by: Max von Hippel <maxvonhippel1996@gmail.com>
Signed-off-by: Max von Hippel <maxvonhippel1996@gmail.com>
Comments below
On 1/6/17 7:02 AM, Max von Hippel wrote:
This PR fixes the issue identified by @sishijing <https://github.com/sishijing>
(#61 <#61>) where if you try to use
both command line args and a YAML file the command line args are ignored. It
prioritizes command line args over YAML, so if you set a value in both YAML and
the command line, the command line value will be the one used. This makes sense
to me since if you set a value explicitly in the command line it seems likely
that is the value you want and you probably just forgot to remove the other
value from the YAML.
Excellent and thanks Max. I agree that this is the way to go.
I also set the default values for prior means and starting carbonicity to 0.38
per @tedvh <https://github.com/tedvh>'s request *in the Settings.cpp ternaries*.
I am not sure how best to set these in the actual cluster definition. They
appear to be right already in the included default YAML file, and reading
Cluster.cpp and Cluster.hpp, I don't see a clear way to set these defaults in
their original definition. That said, if you have a suggestion for where to
look, let me know and I'll give it a shot.
Not sure where else carbonicity might be. My guess is that is everywhere that
it needs to be set. Elliot can confirm when he looks at and merges in your
commits.
thanks!
-Ted
…
--------------------------------------------------------------------------------
You can view, comment on, or merge this pull request online at:
#62
Commit Summary
* added yaml code to ask for missing info
* responded to Travis's comments RE my last PR w/ imports, stream, etc.
Cleaned up code accordingly and tested. Removed compiled code. Not sure how
to add binaries to a .gitignore but I think it would be a good idea to do so.
* Work on the plane responding to comments
* Address issue with combining YAML and command line
* Work on the plane responding to comments
* Address issue with combining YAML and command line
* Merge branch 'master' of https://github.com/maxvonhippel/base
File Changes
* *M* base9/Settings.cpp
<https://github.com/argiopetech/base/pull/62/files#diff-0> (247)
Patch Links:
* https://github.com/argiopetech/base/pull/62.patch
* https://github.com/argiopetech/base/pull/62.diff
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#62>, or mute the thread
<https://github.com/notifications/unsubscribe-auth/AEOeMuqQMzmo8rxx2nu8DOAwLhZTQ_zcks5rPebygaJpZM4Lcdfx>.
|
Signed-off-by: Max von Hippel <maxvonhippel1996@gmail.com>
Thanks @argiopetech . That said, do you disagree that we should have the ternaries like in this commit? Because right now (as I said in my comments above) the command line args are ignored if the same argument appears in the YAML, and it seems like the command line should be prioritized over the YAML. |
I forgot to ask when this came up what ternaries are.
The goal was to have command-line args supercede anything in the yaml file.
…On 1/7/17 4:30 PM, Max von Hippel wrote:
Thanks @argiopetech <https://github.com/argiopetech> . That said, do you
disagree that we should have the ternaries like in this commit? Because right
now (as I said in my comments above) the command line args are ignored if the
same argument appears in the YAML, and it seems like the command line should be
prioritized over the YAML.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#62 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AEOeMjAm4_SCoggebcbXGGbQ2YXsqRRHks5rP72ggaJpZM4Lcdfx>.
|
We actually have a much cleaner method of handling this already in the code. You'll note that every call to settings.fromCLI (argc, argv);
if (!settings.files.config.empty())
{
settings.fromYaml (settings.files.config);
}
else
{
settings.fromYaml ("base9.yaml");
}
settings.fromCLI (argc, argv); The first call to |
I do think that this could be encapsulated into one function void Settings::loadSettings(int argc, char* argv[], const std::string& defaultFile="base9.yaml")
{
fromCLI (argc, argv);
if (!files.config.empty())
{
fromYaml (files.config);
}
else
{
fromYaml (defaultFile);
}
fromCLI (argc, argv);
} so we could just call The only reason it isn't this way already is because it was implemented piece by piece historically (CLI functionality came after YAML files), and it hasn't been broken since then (so I haven't changed it). |
Ternaries are the if/then/else format formatted as such:
which is equivalent to
It's just a shortcut, but if overused or poorly formatted they can become very hard to read. I prefer to reserve them for conditional assignment, and I use them to this end several places in the code for brevity. As such, I have no issues with the way @maxvonhippel has used them here.
This has been the case since the CLI functionality was implemented. The only reason it wasn't working for Shijing was that one of the command line flags he was using wasn't recognized because I had failed to implement it. |
I went ahead and implemented the |
You're right that's infinitely more elegant. Definitely feel foolish now for writing a billion ternary statements when I could have just switched the order of the settings. |
That said, we should change the getOrRequest behavior so that it checks YAML, checks |
gotcha on both items. thanks.
…On 1/7/17 5:02 PM, Elliot Robinson wrote:
I forgot to ask when this came up what ternaries are.
Ternaries are the if/then/else format formatted as such:
|<conditional> ? <do if true> : <do if false>
|
which is equivalent to
|if <conditional>
{
<do if true>
}
else
{
<do if false>
}
|
It's just a shortcut, but if overused or poorly formatted they can become very
hard to read. I use them several places in the code for brevity, and I have no
issues with the way @maxvonhippel <https://github.com/maxvonhippel> has used
them here.
The goal was to have command-line args supercede anything in the yaml file.
This has been the case since the CLI functionality was implemented. The only
reason it wasn't working for Shijing was that one of the command line flags he
was using wasn't recognized because I had failed to implement it.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#62 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AEOeMgV7rhxqrHiyYpCuVrsbnLVdmwatks5rP8USgaJpZM4Lcdfx>.
|
Sorry, on my iPhone. So it checks YAML, then command line, then asks for anything that's missing. Rather than YAML, ask for missing stuff, then command line. |
No worries, a big part of that was reliant on my latency... Apologies again. Had I gotten to you before the code we could have nipped this in the bud.
Ted may be a better person to ask than me on this one. I'm of the opinion that the |
On 1/7/17 7:42 PM, Elliot Robinson wrote:
That said, we should change the getOrRequest behavior so that it ... checks
YAML, then command line, then asks for anything that's missing. Rather than
YAML, ask for missing stuff, then command line.
Ted may be a better person to ask than me on this one. I'm of the opinion that
the |getOrRequest| functionality is for cases where we (the code maintainers)
have updated the YAML specification but the end user has not updated their local
YAML file. The case where an end user has overridden a required YAML setting
from the command line without updating their YAML file should never happen.
Adding logic to handle this potential edge case seems excessive to me.
This is an interesting one. I hadn't thought of what you are saying Elliot,
i.e. this is to fix out-of-date yaml files with (now) missing entries.
I'm not sure what you (Elliot) mean by "The case where an end user has
overridden a required YAML setting from the command line without updating their
YAML file should never happen". If you mean that I might specify a parameter in
the YAML file, but then run BASE9 with a different value for that same parameter
from the command line, that will indeed happen. I do that when I want to run
the same data set through BASE9 N times with different parameters and just have
a script do this with changing command line arguments.
This also raises an interesting issue. Can we easily let users know that their
yaml file is out of date? In the case you mention, Elliot, where BASE9 wants a
parameter that isn't even listed in the yaml file, would there be an easy way to
recognize that and throw a warning? Not a stop, just a warning?
good discussions!
…-Ted
|
I mean that if I add a new parameter Basically, if that last thing were ever to happen, it's probably a sign that you have used an old version of your YAML file.
That's pretty much what Max's code does now, in that it allows you to enter a value for that missing parameter and continue. |
Hi Elliot -
OK, it sounds like your order (yaml, prompt, command-line) does make sense,
though that wasn't intuitive to me either. I'd suggest then, that if it isn't
too cumbersome, Max's update could warn the user that the missing item has no
entry in the yaml file. Maybe that is already obvious to Max, though it just
occurred to me.
thanks gents,
-Ted
…On 1/7/17 8:11 PM, Elliot Robinson wrote:
I'm not sure what you (Elliot) mean by "The case where an end user has
overridden a required YAML setting from the command line without updating
their YAML file should never happen". If you mean that I might specify a
parameter in the YAML file, but then run BASE9 with a different value for
that same parameter from the command line, that will indeed happen. I do
that when I want to run the same data set through BASE9 N times with
different parameters and just have a script do this with changing command
line arguments.
I mean that if I add a new parameter |foo| to the YAML file without telling you,
and you therefore run BASE9 without updating your YAML file, it should ask you
for a value for |foo| via Max's |getOrRequest|. There should very seldom be a
case where you don't have |foo| in your YAML file but have used the theoretical
|--foo| flag, such that Max's |getOrRequest| would trigger, but the value you
specify to |getOrRequest| would then be overridden by the |--foo| flag you
passed at the command line.
Basically, if that last thing were ever to happen, it's probably a sign that you
have used an old version of your YAML file.
This also raises an interesting issue. Can we easily let users know that
their yaml file is out of date? In the case you mention, Elliot, where BASE9
wants a parameter that isn't even listed in the yaml file, would there be an
easy way to recognize that and throw a warning? Not a stop, just a warning?
That's pretty much what Max's code does now, in that it allows you to enter a
value for that missing parameter and continue.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#62 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AEOeMs4Its8qNXTTGNsYusMeWDtNwiZmks5rP_GOgaJpZM4Lcdfx>.
|
Done with 3106da6. The updated verbage for
Notably, I've added the clarification for YAML, removes the newline after the colon (I tend to prefer same-line input unless the text line is very long), and uses |
sounds good!
…On 1/7/17 8:26 PM, Elliot Robinson wrote:
Done with 3106da6
<3106da6>.
The updated verbage for |getOrRequest| is:
|No value was found in the YAML file for parameter 'pi'.
Please enter your desired value for this parameter: 3.1415
|
Notably, I've added the clarification for YAML, removes the newline after the
colon (I tend to prefer same-line input unless the text line is very long), and
uses |cerr| instead of |cout| to avoid potential issues with flushing.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#62 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AEOeMiF_zK3S-TqUo7-vI1uRbvJD0P_mks5rP_TcgaJpZM4Lcdfx>.
|
This PR fixes the issue identified by @sishijing (#61) where if you try to use both command line args and a YAML file the command line args are ignored. It prioritizes command line args over YAML, so if you set a value in both YAML and the command line, the command line value will be the one used. This makes sense to me since if you set a value explicitly in the command line it seems likely that is the value you want and you probably just forgot to remove the other value from the YAML.
I also set the default values for prior means and starting carbonicity to 0.38 per @tedvh's request in the Settings.cpp ternaries. I am not sure how best to set these in the actual cluster definition. They appear to be right already in the included default YAML file, and reading Cluster.cpp and Cluster.hpp, I don't see a clear way to set these defaults in their original definition. That said, if you have a suggestion for where to look, let me know and I'll give it a shot.