[pbs-devel] applied: [PATCH] api macro: reuse generated default const for "unwrap_or"

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

applied

On Mon, Oct 26, 2020 at 06:29:27PM +0100, Thomas Lamprecht wrote:
> Instead of setting a default value to a const and inside an
> .unwrap_or_else closure, lets set it only to the const and reuse that
> later in .unwrap_or
> 
> To achieve that we move the "unrwap_or" code for param plumbing code generation
> a bit later so that we have easy access to the generated const name.
> As all this code is related to optional/default-value stuff it does read still
> relatively OK with that change, IMO.
> 
> This has the advantage of not getting a warning like:
> 
> >  warning: constant is never used: `API_METHOD_EXAMPLE_FOO_PARAM_DEFAULT_FORCE`
> >   --> src/api2/node/foo.rs
> >    |
> > XY |             force: {
> >    |             ^^^^^
> >    = note: `#[warn(dead_code)]` on by default
> 
> When one has a API endpoint like:
> 
> > #[api(
> >     input: {
> >         properties: {
> >             force: {
> >                 type: bool,
> >                 optional: true,
> >                 default: false,
> >             },
> >         },
> >     },
> >     ...
> > )]
> > /// Example
> > fn example_foo(force: bool) -> Result<(), Error> {
> >     if force {
> >         // do something
> >     }
> >     Ok(())
> > }
> 
> It effectively changes the output for optional parameters with a default set
> and no Option<T> from
> 
> > let p = p.unwrap_or_else(|| #default_value);
> 
> to
> 
> > let p = p.unwrap_or(#const_name_for_default);
> 
> where the "#const_name_for_default" is a pub const with value
> "#default_value"
> 
> Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
> ---
> 
> So, I did not checked the surrounding stuff in depth, but it seemed relatively
> clear what is happening here. So, while a good double check is surely good, and
> soem implementation details may be discussed, it does not breaks tests (and I
> negative checked that, to ensure there are tests for this) plus my use case
> works too, the "constant is never used" warning goes away.
> 
>  proxmox-api-macro/src/api/method.rs | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/proxmox-api-macro/src/api/method.rs b/proxmox-api-macro/src/api/method.rs
> index e94594f..1d32c60 100644
> --- a/proxmox-api-macro/src/api/method.rs
> +++ b/proxmox-api-macro/src/api/method.rs
> @@ -414,18 +414,8 @@ fn create_wrapper_function(
>                              #name_str,
>                          ))?
>                      });
> -                } else if util::is_option_type(ty).is_none() {
> -                    // Optional parameter without an Option<T> type requires a default:
> -                    if let Some(def) = &default_value {
> -                        body.extend(quote_spanned! { span =>
> -                            .unwrap_or_else(|| #def)
> -                        });
> -                    } else {
> -                        bail!(ty => "Optional parameter without Option<T> requires a default");
> -                    }
>                  }
> -                body.extend(quote_spanned! { span => ; });
> -                args.extend(quote_spanned! { span => #arg_name, });
> +                let no_option_type = util::is_option_type(ty).is_none();
>  
>                  if let Some(def) = &default_value {
>                      let name_uc = name.as_ident().to_string().to_uppercase();
> @@ -438,7 +428,17 @@ fn create_wrapper_function(
>                      default_consts.extend(quote_spanned! { span =>
>                          pub const #name: #ty = #def;
>                      });
> +                    if optional && no_option_type {
> +                        // Optional parameter without an Option<T> type requires a default:
> +                        body.extend(quote_spanned! { span =>
> +                            .unwrap_or(#name)
> +                        });
> +                    }
> +                } else if optional && no_option_type {
> +                    bail!(ty => "Optional parameter without Option<T> requires a default");
>                  }
> +                body.extend(quote_spanned! { span => ; });
> +                args.extend(quote_spanned! { span => #arg_name, });
>              }
>          }
>      }
> -- 
> 2.27.0