Open yasirroni opened 2 years ago
Thanks for your feedback @yasirroni. This is very helpful.
I'll offer my responses in reverse order ...
3. This was a simple logic bug, fixed by moving the rmpath
inside the conditional that checks for existing installed MATPOWER components.
2. Internally, an interactive
flag is turned on only if install_matpower
is called without any arguments, in which case verbose
defaults to 1. However, there was one place, after removing old paths, where I forgot to check the interactive
flag before pausing for user input. With that fixed, I don't think there is should be a need for 3 levels of verbose.
1. The current design includes the following options (combinations of modify
and save_it
):
(modify, save_it)
(0, 0) - print ADDPATH commands only
(0, 1) - save ADDPATH commands to 'startup.m'
(0, 'name') - save ADDPATH commands to 'name.m'
(1, 0) - execute ADDPATH commands without SAVEPATH
(1, 1) - execute ADDPATH commands with SAVEPATH
It seems to me that your proposal elminates the 3rd option, correct? And I believe it also introduces the possibility of at least one combination of inputs that doesn't make sense, namely modify
= 0 and save_it
= 2 (save the path without modifying it). Would just updating the help text with the list of combinations above clarify things sufficiently? Or do you still think there is a need to redefine this parameter?
Finally, besides the ability to uninstall an old MATPOWER permanently, without the need for a manual savepath
, is there anything else install_matpower
should be able to do and can't (with the fixes mentioned in 2 and 3 above)?
To address the permanent uninstall option, I'm thinking of possibly getting rid of the 4th argument and changing the first argument modify
to have the following options:
0 - print and/or save ADDPATH commands, but don't execute them
1 - install MATPOWER (execute ADDPATH)
2 - uninstall existing MATPOWER (execute RMPATH)
3 - uninstall existing MATPOWER (execute RMPATH), then install MATPOWER (execute ADDPATH)
What would you think of that?
- This was a simple logic bug, fixed by moving the rmpath inside the conditional that checks for existing installed MATPOWER components.
Good. I don't know simple fix will solve it.
However, there was one place, after removing old paths, where I forgot to check the interactive flag before pausing for user input. With that fixed, I don't think there is should be a need for 3 levels of verbose.
I'm supporting this. More value will increase complexity anyway.
- The current design includes ...
My proposal is to support uninstall and save. But, (0, 1, 0, 0) is not possible to do it. It instead make:
(0, 1) - save ADDPATH commands to 'startup.m'
0 - print and/or save ADDPATH commands, but don't execute them 1 - install MATPOWER (execute ADDPATH) 2 - uninstall existing MATPOWER (execute RMPATH) 3 - uninstall existing MATPOWER (execute RMPATH), then install MATPOWER (execute ADDPATH)```
I'm supporting this. The main point of my issue is to support proper uninstall.
I have lots of issue with this
install_matpower.m
(tested on Octave only).SAVE_IT
: It's too confusing with its relation toMODIFY
. How about make it simple such as2
forSAVEPATH
,1
for 'startup.m', and0
for no save at all.VERBOSE
: Verbose0
should silent all message, including interactivity. Maybe 3 level of verbose:2
for interactive,1
for prints only, and0
to no feedback at all.RM_OLDPATHS
: Remove old path when there is no matpower throw Error. We should just return a warning since native rmpath will skip any non existing path, not necessarily throw error. See notes for error message.By updating 1 and 2, we can use
install_matpower
to uninstall matpower without a need for user to manually typesavepath
.Note:
The error message: