Re: [pve-devel] [PATCH common v5 1/7] tools: add check_list_empty function

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

The subject still talks about the old name from v4, but..

Am 02.10.24 um 15:11 schrieb Aaron Lauterer:
> In some situations we don't want a total empty list. I opted for a
> dedicated function instead of integrating it as error in the
> `split_list` function. It is used in many places and the potential
> fallout from unintended behavior changes is too big.
> 
> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
> ---
> changes since:
> v4: rename to `list_is_empty` and switch the return values
> v3: none
> v2: newly added
> 
>  src/PVE/Tools.pm | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/src/PVE/Tools.pm b/src/PVE/Tools.pm
> index bd305bd..36ffb05 100644
> --- a/src/PVE/Tools.pm
> +++ b/src/PVE/Tools.pm
> @@ -718,6 +718,14 @@ sub split_list {
>      return @data;
>  }
>  
> +sub list_is_empty {
> +    my ($list) = @_;
> +    if (scalar(PVE::Tools::split_list($list)) < 1) {

(You're already in the PVE::Tools module, so this could call split_list directly)

> +	return 1;
> +    }
> +    return 0;

... this saves the call site a not really much, i.e. it would one allow to
write something like:

if (scalar(PVE::Tools::split_list($list)) == 0) {
    # ...
}

if (PVE::Tools::list_is_empty($list))) {
    # ...
}

And don't get me wrong, having a dedicated helper for such things that then
semantically encodes what's checked for in the method name can be fine, but
PVE::Tools is already very bloated, and it doesn't seem like we already require
this pattern often, or did you found other sites where this could be used?

Once pve-common gets split up, and we got some more specific and smaller module
dedicated to such stuff it might be fine to add this helper there, but for now
I'd avoid adding this as long as there are not many (existing) use cases.

I could fix patch 6/7 up on applying if nothing else pops up to avoid that
you need to send a v6 just for this.

> +}
> +
>  sub trim {
>      my $txt = shift;
>  



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel