From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 92403E508 for ; Fri, 9 Dec 2022 10:22:34 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 7212321E2E for ; Fri, 9 Dec 2022 10:22:04 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Fri, 9 Dec 2022 10:22:03 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 4E140441C7 for ; Fri, 9 Dec 2022 10:22:03 +0100 (CET) Date: Fri, 9 Dec 2022 10:22:01 +0100 From: Wolfgang Bumiller To: Lukas Wagner Cc: pbs-devel@lists.proxmox.com Message-ID: <20221209092201.v7zb4tbkzg3niyns@wobu-vie.proxmox.com> References: <20221207093819.75847-1-l.wagner@proxmox.com> <20221207093819.75847-5-l.wagner@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20221207093819.75847-5-l.wagner@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.224 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pbs-devel] [PATCH v3 proxmox-backup 4/4] debug cli: move parameters into the function signature X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 09 Dec 2022 09:22:34 -0000 On Wed, Dec 07, 2022 at 10:38:19AM +0100, Lukas Wagner wrote: > Signed-off-by: Lukas Wagner > --- > 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, 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, > + ns: Option, ^ 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 { > - 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>, > -- > 2.30.2