Re: [PATCH 2/8] zone: accept also hids in zone_config()

Hi,

On Tue, 2017-07-25 at 18:07 +0200, Jonatan Schlag wrote:

Signed-off-by: Jonatan Schlag jonatan.schlag@ipfire.org

src/functions/functions.zone | 33 ++++++++++++++++++++++++---------  1 file changed, 24 insertions(+), 9 deletions(-)

diff --git a/src/functions/functions.zone b/src/functions/functions.zone index 28a22fe..091bcd6 100644 --- a/src/functions/functions.zone +++ b/src/functions/functions.zone @@ -550,28 +550,43 @@ zone_config() {   zone_config_new "${zone}" "$@"   ;;   destroy)

• 	# id could be also a valid hid
  

local id=${1}

• 	if zone_config_id_is_valid ${zone} ${id}; then
  

• 	# usually we will get a hid so we check first if we
  

get one

• 	if zone_config_hid_is_valid ${zone} ${id}; then
  

• 		# Convert the hid into an id
  

• 		id=$(zone_config_convert_hid_to_id ${zone}
  

${id})   zone_config_destroy "${zone}" "$@"

Could we not just call "zone_config_convert_hid_to_id" (maybe the name would be better without "convert") and if it does not return an ID, we assume that the HID does not exist?

That would save us running through all the hooks twice which is quite an expensive thing to do.

else

• 		log ERROR "${id} is not a valid id"
  

• 		# We get no valid hid so we check if we get a
  

valid id

• 		if zone_config_id_is_valid ${zone} ${id};
  

then

• 			zone_config_destroy "${zone}" "$@"
  

• 		else
  

• 			log ERROR "${id} is not a valid id or
  

hid"

• 		fi
  

fi   ;;   list)   zone_config_list "${zone}" "$@"   ;;   *)

• 	# Check is we get a valid id
  

• 	# TODO This could be also a valid hid
  

• 	# id could be also a valid hid
  

local id=${cmd}

• 	if zone_config_id_is_valid ${zone} ${id} && [[ ${1}
  

== "edit" ]]; then

• 	# usually we will get a hid so we check first if we
  

get one

• 	if zone_config_hid_is_valid ${zone} ${id} && [[ ${1}
  

== "edit" ]]; then

• 		id=$(zone_config_convert_hid_to_id ${zone}
  

${id})

Can we also remove this ${1} == edit thing. That has confused me quite a few times and doesn't make the code very obvious to read.

But same thing here as above. Could be something like:

local id = $(hid to id ${hid} if ! isset id; then ... check if HID is an ID fi

... assume that we have a valid ID from here on ...

This would have a little bit of a performance penalty if the user has given an ID or an invalid HID. But I think it is rather likely that someone will give a HID than an ID. Mainly because auto-completion will give a HID instead of an ID.

shift 1   zone_config_edit "${zone}" "${id}" "$@"   else

• 		error "Unrecognized argument: ${cmd}"
  

• 		cli_usage root-zone-config-subcommands
  

• 		exit ${EXIT_ERROR}
  

• 		# We get no valid hid so we check if we get a
  

valid id

• 		if zone_config_id_is_valid ${zone} ${id} &&
  

[[ ${1} == "edit" ]]; then

• 			shift 1
  

• 			zone_config_edit "${zone}" "${id}"
  

"$@"

• 		else
  

• 			error "Unrecognized argument: ${cmd}"
  

• 			cli_usage root-zone-config-
  

subcommands

• 			exit ${EXIT_ERROR}
  

• 		fi
  

fi   ;;   esac