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
Use meaningful variable names for automated UserInput #1473
Use meaningful variable names for automated UserInput #1473
Conversation
…t and require -I option in UserInput calls
@gozora UserInput -I BORGBACKUP_archive_to_recover ... but I think the user input there is run in a loop where the user UserInput -I BORGBACKUP_archive_to_recover_1 ... UserInput -I BORGBACKUP_archive_to_recover_2 ... UserInput -I BORGBACKUP_archive_to_recover_3 ... ... so that the user can specify as many BorgBackup archives UserInput_BORGBACKUP_archive_to_recover_1="archive1" UserInput_BORGBACKUP_archive_to_recover_1="archive2" UserInput_BORGBACKUP_archive_to_recover_1="archive3" ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that this is much easier - thanks!
Can you please lowercase or uppercase all IDs? As Bash variables are case sensitive I think that we should make it easier for our users so that they don't have to remember the capitalization of the variable names.
Since the ID value is now a mandatory parameter I strongly suggest to remove the -I
from it and to make it simply mandatory to provide this ID as the first parameter to the UserInput
function.
Sorry (really truly sorry) that I realized it only now: The UserInput
function should be named user_input
according to https://github.com/rear/rear/wiki/Coding-Style#functions
The fact that the other input/output functions are camel case is probably more of a legacy issue and less of a recommendation. I think is is OK to add new stuff with the new lower cased spelling and to migrate existing stuff when we refactor it for other reasons.
@schabrolles UserInput -I multipath_failed_to_list_device ... Please comment if that '-I user_input_ID' option value |
Intentionally I named the UserInput and my other new functions |
I think it is misleading to lowercase things that are used Perhaps BORGBACKUP should be even correctly None of the non-lowercase spellings is my own spelling. |
Hello @jsmeix,
Wrong ;-), and thank you for teaching me how comments can be useful ;-) Reading comment:
So that loop is there only for user to choose archive to restore or exit if desired archive is missing, no other fancy functionalities. V. |
@gozora |
Sorry @jsmeix I disagree. IMHO mixed spelling is the worst. So within a single name or ID the spelling should be consistent. Either uppercase like in global variables or lowercase as in function names. Good:
Bad:
I am also fine with uppercasing all the user input ID names. This is consistent with our coding style to upper case global variables. Yes, there is a lot of old mess in ReaR. That is why I believe it to be better to introduce new stuff following our new guidelines instead of matching new stuff to the wrong ways of the past.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it!
@schlomo just to ease my curiosity (really have no preference in naming and I'm still looking for "perfect" formatting style), why do you think Set_size_for_EFI_boot_in_MiB is bad ? |
@gozora the main reason that is bad is that you have an additional dimension of potential errors. As a user you not only have to get the words right you on top of that also have to get the spelling right. IMHO this is a totally needles source of errors that doesn't help anybody. If we want to force users to always copy-paste those IDs then we should be using random IDs or UUIDs instead. If we want users to be able to just type them in then we should use a very consistent and as easy as possible way of writing those variables. |
…meant as user config variables
Thanks a lot @jsmeix I know that this is a lot of work. |
… and now copy all output_array elements
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jsmeix I trust you with the changes. Perhaps also update the CodingStyle with the new DebugPrint and other goodies if required.
Thanks for the hard work
In shameless self-praise mode I am impressed: Welcome to Relax-and-Recover. Run "rear recover" to restore your system ! RESCUE e205:~ # export USER_INPUT_LAYOUT_MIGRATION_REPLACEMENT_DISK="1" RESCUE e205:~ # export USER_INPUT_LAYOUT_MIGRATION_CONFIRM_MAPPINGS="yEs" RESCUE e205:~ # rear recover ... Comparing disks. Device sda has size 22548578304, 21474836480 expected Switching to manual disk layout configuration. Original disk /dev/sda does not exist (with same size) in the target system Choose an appropriate replacement for /dev/sda 1) /dev/sda 2) /dev/sdb 3) Do not map /dev/sda 4) Use Relax-and-Recover shell and return back to here (default 1 timeout 300 seconds) UserInput: Will use predefined input in 'USER_INPUT_LAYOUT_MIGRATION_REPLACEMENT_DISK'='1' Hit any key to interrupt the automated input (timeout 5 seconds) Using /dev/sda (chosen by user) for recreating /dev/sda Current disk mapping table (source -> target): /dev/sda /dev/sda Confirm or edit the disk mapping 1) Confirm disk mapping and continue 'rear recover' 2) Edit disk mapping (/var/lib/rear/layout/disk_mappings) 3) Use Relax-and-Recover shell and return back to here 4) Abort 'rear recover' (default 1 timeout 300 seconds) UserInput: Will use predefined input in 'USER_INPUT_LAYOUT_MIGRATION_CONFIRM_MAPPINGS'='1' Hit any key to interrupt the automated input (timeout 5 seconds) User confirmed disk mapping ... |
Cool! how come that |
…ut of the while loop
Because I coded it in layout/prepare/default/300_map_disks.sh # When USER_INPUT_LAYOUT_MIGRATION_CONFIRM_MAPPINGS has any 'true' value be liberal in what you accept and # assume choices[0] 'Confirm mapping' was actually meant which is shown with choice number 1 (not 0) and # a predefined user input must match the choice number that is shown to the user (not the choice index): is_true "$USER_INPUT_LAYOUT_MIGRATION_CONFIRM_MAPPINGS" && USER_INPUT_LAYOUT_MIGRATION_CONFIRM_MAPPINGS="1" and similar things in |
I made For now I think this pull request is good enough to be merged. The main issue was that variables for predefined user input The names of user input/output functions can be changed I prefer consistent function names |
I see, nice trick. I think that this is a pattern that you might want to add to our coding docs as it is not obvious and it requires the developer to perceive the user input variable as a multi-value variable that can be either true or a number from the list. It also forces the developer to have the first list item as the "true" value. |
I think that you should slowly get to merge this so that we can collect some feedback from users. About backwards compatible: In general I agree, in particular I like to understand where the backwards compatibility creates a lot of complexity in ReaR. In those cases I would like us to discuss a breaking change, especially if it something where we learned over time that previous assumptions were less than optimum. |
@schlomo I am strongly :-) against positional parameters/arguments I strongly ;-) suggest to prefer named parameters/arguments If the ID parameter was a positional parameter I could not UserInput FOO BAR BAZ where FOO is meant as ID and BAR BAZ are choices UserInput FOO BAR BAZ where no ID is specified because FOO BAR BAZ are |
@jsmeix very good reasoning which convinces me, you probably also would love programming in Python 😄 |
@schabrolles # TODO: Currently only one single # USER_INPUT_LAYOUT_MIGRATION_REPLACEMENT_DISK # can be predefined (which is at least better # than nothing) but that dialog can appear # several times for several unmapped # original 'disk' devices and 'multipath' devices: |
FYI: |
#1486 It was unexpectedly easy :-) |
Now each UserInput function call requires
a specified '-I user_input_ID' option and
the autogenerated user_input_ID is gone.
Now the user_input_ID option value can and must be
a valid bash variable name e.g. 'choose_replacement_disk'
so that the user can (if he wants) specify a variable named
UserInput_choose_replacement_disk
(i.e. the user_input_ID option value with prefix 'UserInput_')
where its value is used as automated input.
This way meaningful variable names for automated UserInput
are implemented in a generic way using plain bash syntax
without any restriction how the user needs to specify
his particular automated input settings.
The user can do this statically in his local.conf like
UserInput_choose_replacement_disk="/dev/sda"
or via whatever sophisticated scripting magic as he likes
(e.g. also local.conf is sourced and executed as a script).