Closed LuminousPath closed 3 years ago
It will disallow overriding the variables via the function arguments. We will need to put the code around here https://github.com/randy3k/Terminus/blob/master/terminus/core.py#L148
you can't check for whether cmd
and shell_cmd
exist before they're potentially assigned values, as it stands now, there are potential occasions when both cmd
and shell_cmd
are both null and this function doesn't work as intended.
I can add some gating if not cmd
and if not shell_cmd
lines to allow for overridding, but you have to put the assignment earlier in the function to be able to use shell_cmd
the way that its written out here.
I'll make my proposed changes to the PR and you can take a look :)
Unfortunately, it won't work either. The not cmd
in https://github.com/LuminousPath/Terminus/blob/patch-1/terminus/core.py#L141 will never be triggered because cmd
has a fallback value.
We need to refactor the logic a little bit. Maybe also to rename some variables to avoid confusion.
Let a_xxx
be the values from the arguments and c_xxx
be the values from the config.
There are four variables a_cmd
, a_shell_cmd
, c_cmd
, c_shell_cmd
. Note that the default config and most other configs have non empty values of c_cmd
.
The rules are:
a_cmd
or a_shell_cmd
is non empty, ignore both c_cmd
and c_shell_cmd
.a_cmd
and a_shell_cmd
are specifiedc_cmd
and c_shell_cmd
are specifieda_cmd
dominates c_cmd
and a_shell_cmd
dominates c_shell_cmd
.The following table lists the expected results.
a_cmd | a_shell_cmd | c_cmd | c_shell_cmd | result |
---|---|---|---|---|
0 | 0 | 0 | 0 | error: need cmd or shell_cmd but not both |
0 | 0 | 0 | 1 | c_shell_cmd |
0 | 0 | 1 | 0 | c_cmd |
0 | 0 | 1 | 1 | error: need cmd or shell_cmd but not both |
0 | 1 | 0 | 0 | error: need cmd or shell_cmd but not both |
0 | 1 | 0 | 1 | a_shell_cmd |
0 | 1 | 1 | 0 | a_shell_cmd |
0 | 1 | 1 | 1 | error: need cmd or shell_cmd but not both |
1 | 0 | 0 | 0 | error: need cmd or shell_cmd but not both |
1 | 0 | 0 | 1 | a_cmd |
1 | 0 | 1 | 0 | a_cmd |
1 | 0 | 1 | 1 | error: need cmd or shell_cmd but not both |
1 | 1 | 0 | 0 | error: need cmd or shell_cmd but not both |
1 | 1 | 0 | 1 | error: need cmd or shell_cmd but not both |
1 | 1 | 1 | 0 | error: need cmd or shell_cmd but not both |
1 | 1 | 1 | 1 | error: need cmd or shell_cmd but not both |
I see what you're getting at, how does the following construct sound:
cmd = cmd or config["cmd"] if "cmd" in config else None
shell_cmd = shell_cmd or config["shell_cmd"] if "shell_cmd" in config else None
if not (bool(cmd) ^ bool(shell_cmd)):
raise ValueError("need cmd or shell_cmd but not both")
Added lines at beginning of
run_async
to assign variables tocmd
andshell_cmd
if they exist in the config. In response to #248