mbox

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

Message ID 1500998833-10543-3-git-send-email-jonatan.schlag@ipfire.org
State New
Headers

Message

Jonatan Schlag July 26, 2017, 2:07 a.m. UTC 
  Signed-off-by: Jonatan Schlag <jonatan.schlag@ipfire.org>
---
 src/functions/functions.zone | 33 ++++++++++++++++++++++++---------
 1 file changed, 24 insertions(+), 9 deletions(-)

Comments

Michael Tremer July 26, 2017, 8:27 a.m. UTC | #1 
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