GNOME Bugzilla – Bug 707576
[review] nmcli: add 'remove' command to nmcli interactive editor
Last modified: 2013-09-09 06:36:43 UTC
Add 'remove' command for the main menu. It allows removing settings from the connection, or deleting (resetting) property values. This is important, e.g. for being able to remove 'wifi-sec' setting. Please review in jklimes/nmcli-remove-cmd. A few side notes: * At present 'set' command resets property value, when empty value is provided. Should we change this to make it noop? * We should consolidate the code of editor_menu_main(), but it will be done later. * We also should enhance TAB-completion, again not part of this bug/branch.
I don't think changing the empty value set to a NOP is really necessary, unless it actually causes problems. One question: would "delete" be a more appropriate command? /sbin/ip uses "del" to add/remove stuff, for example. A few wording changes: + printf (_("remove <setting>[.<prop>] :: remove setting (or property value)\n\n" + "This command removes a setting from connection. If property is given\n" + "its value is removed instead (set to default).\n\n" + "Examples: nmcli> remove wifi-sec\n" + " nmcli> remove eth.mtu\n")); Perhaps: ":: remove setting or reset property value" "This command removes an entire setting from the connection, or if a property is given, resets that property to the default value."
I think it's fine. I second Dan's comment about wording changes. Also at: printf (_("Error: neither invalid property: %s, " "neither a valid setting name.\n"), Also, I attach a patch with minor suggestions.
Created attachment 254204 [details] [review] nmcli-remove-cmd.patch Minor changes, just a suggestion...
Both menu and the error message reworded and pushed: 242bebfb3d7558afcf68fa6b00cabb761b182d4e Dan, it would help if you could proof read also other menu texts. So that it is in understandable English without grammar faults. Thomas, the patch in comment #3 is unrelated, we will solve it separately. Setting nmc_completion_setting should also be moved to menu_switch_to_level1().
(In reply to comment #1) > > One question: would "delete" be a more appropriate command? /sbin/ip uses > "del" to add/remove stuff, for example. > I left it as 'remove' for now. I preferred 'remove' over 'delete' for two reasons: 1. To be consistent with 'remove' in the properties submenu. 2. We have 'describe' command already, and using 'delete' would make a collision for 'd' command. ('r' is unique.)