Re: [pbs-devel] [PATCH v3 proxmox-backup 4/4] debug cli: move parameters into the function signature

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

On Wed, Dec 07, 2022 at 10:38:19AM +0100, Lukas Wagner wrote:
> Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
> ---
> Changes from v2:
>   - This commit is new in v3: `diff_archive_cmd`: Moved parameters into the
>     function signature, instead of manually extracting it from `Value`.
> 
>  src/bin/proxmox_backup_debug/diff.rs | 65 +++++++++++-----------------
>  1 file changed, 25 insertions(+), 40 deletions(-)
> 
> diff --git a/src/bin/proxmox_backup_debug/diff.rs b/src/bin/proxmox_backup_debug/diff.rs
> index 2149720f..3160efb1 100644
> --- a/src/bin/proxmox_backup_debug/diff.rs
> +++ b/src/bin/proxmox_backup_debug/diff.rs
> @@ -25,9 +25,9 @@ use pbs_config::key_config::decrypt_key;
>  use pbs_datastore::dynamic_index::{BufferedDynamicReader, DynamicIndexReader, LocalDynamicReadAt};
>  use pbs_datastore::index::IndexFile;
>  use pbs_tools::crypt_config::CryptConfig;
> -use pbs_tools::json::required_string_param;
>  use pxar::accessor::ReadAt;
>  use pxar::EntryKind;
> +use serde::Deserialize;
>  use serde_json::Value;
>  
>  use termcolor::{Color, ColorChoice, ColorSpec, StandardStream, WriteColor};
> @@ -92,8 +92,7 @@ pub fn diff_commands() -> CommandLineInterface {
>              },
>              "color": {
>                  optional: true,
> -                type: String,
> -                description: "Set mode for colored output. Can be `always`, `auto` or `never`. `auto` will display colors only if stdout is a tty. Defaults to `auto`."
> +                type: ColorMode,
>              }
>          }
>      }
> @@ -102,29 +101,20 @@ pub fn diff_commands() -> CommandLineInterface {
>  /// For modified files, the file metadata (e.g. mode, uid, gid, size, etc.) will be considered. For detecting
>  /// modification of file content, only mtime will be used by default. If the --compare-content flag is provided,
>  /// mtime is ignored and file content will be compared.
> -async fn diff_archive_cmd(param: Value) -> Result<(), Error> {
> +async fn diff_archive_cmd(
> +    prev_snapshot: String,
> +    snapshot: String,
> +    archive_name: String,
> +    compare_content: Option<bool>,

The `#[api]` macro is able to fill in defaults for simple types.

For `compare_content`, you can just drop the `Option<>` and fill in the
`default:` in the `#[api]` macro.

You do want to add `default` to `optional` parameters as much as
possible anyway, since those will be visible in the generated API
documentation.

> +    color: Option<ColorMode>,
> +    ns: Option<BackupNamespace>,

^ These 2 will have to stay an Option for now, since here the API
description is not "visible" to the `#[api]` macro since it cannot
access anything outside its immediate scope.

Basically, whenever `schema: FOO` or `type: Type` is used in `#[api]`,
the `#[api]` macro can do less on its own.
We *could* add the ability to defer to some trait they have to implement
(there's already an `ApiType` trait), but this is not yet implemented
and may be a bit tricky sometimes.

> +    param: Value,
> +) -> Result<(), Error> {
>      let repo = extract_repository_from_value(&param)?;
> -    let snapshot_a = required_string_param(&param, "prev-snapshot")?;
> -    let snapshot_b = required_string_param(&param, "snapshot")?;
> -    let archive_name = required_string_param(&param, "archive-name")?;
> -
> -    let compare_contents = match param.get("compare-content") {
> -        Some(Value::Bool(value)) => *value,
> -        Some(_) => bail!("invalid flag for compare-content"),
> -        None => false,
> -    };
>  
> -    let color = match param.get("color") {
> -        Some(Value::String(color)) => color.as_str().try_into()?,
> -        Some(_) => bail!("invalid color parameter. Valid choices are `always`, `auto` and `never`"),
> -        None => ColorMode::Auto,
> -    };
> -
> -    let namespace = match param.get("ns") {
> -        Some(Value::String(ns)) => ns.parse()?,
> -        Some(_) => bail!("invalid namespace parameter"),
> -        None => BackupNamespace::root(),
> -    };
> +    let compare_content = compare_content.unwrap_or(false);
> +    let color = color.unwrap_or_default();
> +    let namespace = ns.unwrap_or_else(BackupNamespace::root);
>  
>      let crypto = crypto_parameters(&param)?;
>  
> @@ -152,11 +142,11 @@ async fn diff_archive_cmd(param: Value) -> Result<(), Error> {
>      if archive_name.ends_with(".pxar") {
>          let file_name = format!("{}.didx", archive_name);
>          diff_archive(
> -            snapshot_a,
> -            snapshot_b,
> +            &prev_snapshot,
> +            &snapshot,
>              &file_name,
>              &repo_params,
> -            compare_contents,
> +            compare_content,
>              &output_params,
>          )
>          .await?;
> @@ -232,25 +222,20 @@ async fn diff_archive(
>      Ok(())
>  }
>  
> +#[api(default: "auto")]
> +#[derive(Default, Deserialize)]
> +#[serde(rename_all = "lowercase")]
> +/// Color output options
>  enum ColorMode {
> +    /// Always output colors
>      Always,
> +    /// Output colors if STDOUT is a tty and neither of TERM=dumb or NO_COLOR is set
> +    #[default]
>      Auto,
> +    /// Never output colors
>      Never,
>  }
>  
> -impl TryFrom<&str> for ColorMode {
> -    type Error = Error;
> -
> -    fn try_from(value: &str) -> Result<Self, Self::Error> {
> -        match value {
> -            "auto" => Ok(Self::Auto),
> -            "always" => Ok(Self::Always),
> -            "never" => Ok(Self::Never),
> -            _ => bail!("invalid color parameter. Valid choices are `always`, `auto` and `never`"),
> -        }
> -    }
> -}
> -
>  struct RepoParams {
>      repo: BackupRepository,
>      crypt_config: Option<Arc<CryptConfig>>,
> -- 
> 2.30.2