[pbs-devel] [PATCH v3 proxmox-backup 4/4] debug cli: move parameters into the function signature
Wolfgang Bumiller
w.bumiller at proxmox.com
Fri Dec 9 10:22:01 CET 2022
On Wed, Dec 07, 2022 at 10:38:19AM +0100, Lukas Wagner wrote:
> Signed-off-by: Lukas Wagner <l.wagner at 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(¶m)?;
> - let snapshot_a = required_string_param(¶m, "prev-snapshot")?;
> - let snapshot_b = required_string_param(¶m, "snapshot")?;
> - let archive_name = required_string_param(¶m, "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(¶m)?;
>
> @@ -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
More information about the pbs-devel
mailing list