Minor refactoring and cleanup (#1007)

* remove benchmarks - not used for now, will need to be rewritten
* move some CLI settings to a separate struct
* move config saving to a shared function
* simplify zoom level CLI parsing
* rename main.rs to be the same name as generated binary (we can have
multiple bins later)

martin/Cargo.toml

@@ -47,7 +47,7 @@ path = "src/lib.rs"
 
 [[bin]]
 name = "martin"
-path = "src/bin/main.rs"
+path = "src/bin/martin.rs"
 
 [features]
 default = []
@@ -99,8 +99,3 @@ criterion.workspace = true
 ctor.workspace = true
 indoc.workspace = true
 insta = { workspace = true, features = ["yaml"] }
-#test-log = "0.2"
-
-[[bench]]
-name = "sources"
-harness = false

martin/src/args/mod.rs

@@ -6,5 +6,5 @@ mod srv;
 
 pub use connections::{Arguments, State};
 pub use environment::{Env, OsEnv};
-pub use pg::{BoundsCalcType, DEFAULT_BOUNDS_TIMEOUT};
-pub use root::{Args, MetaArgs};
+pub use pg::{BoundsCalcType, PgArgs, DEFAULT_BOUNDS_TIMEOUT};
+pub use root::{Args, ExtraArgs, MetaArgs};

martin/src/args/root.rs

@@ -18,6 +18,8 @@ pub struct Args {
     #[command(flatten)]
     pub meta: MetaArgs,
     #[command(flatten)]
+    pub extras: ExtraArgs,
+    #[command(flatten)]
     pub srv: SrvArgs,
     #[command(flatten)]
     pub pg: Option<PgArgs>,
@@ -41,6 +43,11 @@ pub struct MetaArgs {
     pub watch: bool,
     /// Connection strings, e.g. postgres://... or /path/to/files
     pub connection: Vec<String>,
+}
+
+#[derive(Parser, Debug, Clone, PartialEq, Default)]
+#[command()]
+pub struct ExtraArgs {
     /// Export a directory with SVG files as a sprite source. Can be specified multiple times.
     #[arg(short, long)]
     pub sprite: Vec<PathBuf>,
@@ -80,12 +87,12 @@ impl Args {
             config.mbtiles = parse_file_args(&mut cli_strings, "mbtiles");
         }
 
-        if !self.meta.sprite.is_empty() {
-            config.sprites = FileConfigEnum::new(self.meta.sprite);
+        if !self.extras.sprite.is_empty() {
+            config.sprites = FileConfigEnum::new(self.extras.sprite);
         }
 
-        if !self.meta.font.is_empty() {
-            config.fonts = OptOneMany::new(self.meta.font);
+        if !self.extras.font.is_empty() {
+            config.fonts = OptOneMany::new(self.extras.font);
         }
 
         cli_strings.check()

martin/src/bin/main.rs → martin/src/bin/martin.rs

@@ -1,14 +1,10 @@
-use std::ffi::OsStr;
 use std::fmt::Display;
-use std::fs::File;
-use std::io::Write;
 
 use actix_web::dev::Server;
 use clap::Parser;
 use log::{error, info, log_enabled};
 use martin::args::{Args, OsEnv};
 use martin::srv::{new_server, RESERVED_KEYWORDS};
-use martin::Error::ConfigWriteError;
 use martin::{read_config, Config, IdResolver, Result};
 
 const VERSION: &str = env!("CARGO_PKG_VERSION");
@@ -31,20 +27,7 @@ async fn start(args: Args) -> Result<Server> {
     let sources = config.resolve(IdResolver::new(RESERVED_KEYWORDS)).await?;
 
     if let Some(file_name) = save_config {
-        let yaml = serde_yaml::to_string(&config).expect("Unable to serialize config");
-        if file_name.as_os_str() == OsStr::new("-") {
-            info!("Current system configuration:");
-            println!("\n\n{yaml}\n");
-        } else {
-            info!(
-                "Saving config to {}, use --config to load it",
-                file_name.display()
-            );
-            File::create(file_name.clone())
-                .map_err(|e| ConfigWriteError(e, file_name.clone()))?
-                .write_all(yaml.as_bytes())
-                .map_err(|e| ConfigWriteError(e, file_name.clone()))?;
-        }
+        config.save_to_file(file_name)?;
     } else {
         info!("Use --save-config to save or print Martin configuration.");
     }

martin/src/config.rs

@@ -1,11 +1,13 @@
 use std::collections::HashMap;
+use std::ffi::OsStr;
 use std::fs::File;
 use std::future::Future;
 use std::io::prelude::*;
 use std::path::{Path, PathBuf};
 use std::pin::Pin;
 
 use futures::future::try_join_all;
+use log::info;
 use serde::{Deserialize, Serialize};
 use subst::VariableMap;
 
@@ -17,7 +19,7 @@ use crate::pmtiles::PmtSource;
 use crate::source::{TileInfoSources, TileSources};
 use crate::sprites::SpriteSources;
 use crate::srv::SrvConfig;
-use crate::Error::{ConfigLoadError, ConfigParseError, NoSources};
+use crate::Error::{ConfigLoadError, ConfigParseError, ConfigWriteError, NoSources};
 use crate::{IdResolver, OptOneMany, Result};
 
 pub type UnrecognizedValues = HashMap<String, serde_yaml::Value>;
@@ -110,6 +112,26 @@ impl Config {
 
         Ok(TileSources::new(try_join_all(sources).await?))
     }
+
+    pub fn save_to_file(&self, file_name: PathBuf) -> Result<()> {
+        let yaml = serde_yaml::to_string(&self).expect("Unable to serialize config");
+        if file_name.as_os_str() == OsStr::new("-") {
+            info!("Current system configuration:");
+            println!("\n\n{yaml}\n");
+            Ok(())
+        } else {
+            info!(
+                "Saving config to {}, use --config to load it",
+                file_name.display()
+            );
+            match File::create(&file_name) {
+                Ok(mut file) => file
+                    .write_all(yaml.as_bytes())
+                    .map_err(|e| ConfigWriteError(e, file_name)),
+                Err(e) => Err(ConfigWriteError(e, file_name)),
+            }
+        }
+    }
 }
 
 pub fn copy_unrecognized_config(

martin/src/utils/mod.rs

@@ -1,11 +1,14 @@
 mod cfg_containers;
-mod error;
-mod id_resolver;
-mod utilities;
-mod xyz;
-
 pub use cfg_containers::{OptBoolObj, OptOneMany};
+
+mod error;
 pub use error::*;
+
+mod id_resolver;
 pub use id_resolver::IdResolver;
+
+mod utilities;
 pub use utilities::*;
+
+mod xyz;
 pub use xyz::Xyz;

mbtiles/Cargo.toml

@@ -50,7 +50,7 @@ path = "src/lib.rs"
 
 [[bin]]
 name = "mbtiles"
-path = "src/bin/main.rs"
+path = "src/bin/mbtiles.rs"
 required-features = ["cli"]
 
 # Lints inheritance from workspace is not yet supported

mbtiles/src/bin/main.rs → mbtiles/src/bin/mbtiles.rs

@@ -252,7 +252,7 @@ mod tests {
     #[test]
     fn test_copy_zoom_levels_arguments() {
         let mut opt = MbtilesCopier::new(PathBuf::from("src_file"), PathBuf::from("dst_file"));
-        opt.zoom_levels.extend(&[1, 3, 7]);
+        opt.zoom_levels.extend(&[3, 7, 1]);
         assert_eq!(
             Args::parse_from([
                 "mbtiles",

mbtiles/src/copier.rs

@@ -2,7 +2,7 @@ use std::collections::HashSet;
 use std::path::PathBuf;
 
 #[cfg(feature = "cli")]
-use clap::{builder::ValueParser, error::ErrorKind, Args, ValueEnum};
+use clap::{Args, ValueEnum};
 use enum_display::EnumDisplay;
 use log::{debug, info};
 use sqlite_hashes::rusqlite;
@@ -53,8 +53,8 @@ pub struct MbtilesCopier {
     #[cfg_attr(feature = "cli", arg(long, conflicts_with("zoom_levels")))]
     pub max_zoom: Option<u8>,
     /// List of zoom levels to copy
-    #[cfg_attr(feature = "cli", arg(long, value_parser(ValueParser::new(HashSetValueParser{})), default_value=""))]
-    pub zoom_levels: HashSet<u8>,
+    #[cfg_attr(feature = "cli", arg(long, value_delimiter = ','))]
+    pub zoom_levels: Vec<u8>,
     /// Compare source file with this file, and only copy non-identical tiles to destination.
     /// It should be later possible to run `mbtiles apply-diff SRC_FILE DST_FILE` to get the same DIFF file.
     #[cfg_attr(feature = "cli", arg(long, conflicts_with("apply_patch")))]
@@ -68,38 +68,6 @@ pub struct MbtilesCopier {
     pub skip_agg_tiles_hash: bool,
 }
 
-#[cfg(feature = "cli")]
-#[derive(Clone)]
-struct HashSetValueParser;
-
-#[cfg(feature = "cli")]
-impl clap::builder::TypedValueParser for HashSetValueParser {
-    type Value = HashSet<u8>;
-
-    fn parse_ref(
-        &self,
-        _cmd: &clap::Command,
-        _arg: Option<&clap::Arg>,
-        value: &std::ffi::OsStr,
-    ) -> Result<Self::Value, clap::Error> {
-        let mut result = HashSet::<u8>::new();
-        let values = value
-            .to_str()
-            .ok_or(clap::Error::new(ErrorKind::ValueValidation))?
-            .trim();
-        if !values.is_empty() {
-            for val in values.split(',') {
-                result.insert(
-                    val.trim()
-                        .parse::<u8>()
-                        .map_err(|_| clap::Error::new(ErrorKind::ValueValidation))?,
-                );
-            }
-        }
-        Ok(result)
-    }
-}
-
 #[derive(Clone, Debug)]
 struct MbtileCopierInt {
     src_mbtiles: Mbtiles,
@@ -113,7 +81,7 @@ impl MbtilesCopier {
         Self {
             src_file: src_filepath,
             dst_file: dst_filepath,
-            zoom_levels: HashSet::new(),
+            zoom_levels: Vec::default(),
             dst_type_cli: None,
             dst_type: None,
             on_duplicate: CopyDuplicateMode::Override,
@@ -579,13 +547,11 @@ impl MbtileCopierInt {
         let mut query_args = vec![];
 
         let sql = if !&self.options.zoom_levels.is_empty() {
-            for z in &self.options.zoom_levels {
+            let zooms: HashSet<u8> = self.options.zoom_levels.iter().copied().collect();
+            for z in &zooms {
                 query_args.push(*z);
             }
-            format!(
-                " AND zoom_level IN ({})",
-                vec!["?"; self.options.zoom_levels.len()].join(",")
-            )
+            format!(" AND zoom_level IN ({})", vec!["?"; zooms.len()].join(","))
         } else if let Some(min_zoom) = self.options.min_zoom {
             if let Some(max_zoom) = self.options.max_zoom {
                 query_args.push(min_zoom);

mbtiles/tests/mbtiles.rs

@@ -248,7 +248,7 @@ async fn convert(
 
     let mut opt = copier(&frm_mbt, &mem);
     opt.dst_type_cli = Some(dst_type);
-    opt.zoom_levels.insert(6);
+    opt.zoom_levels.push(6);
     let z6only = dump(&mut opt.run().await?).await?;
     assert_snapshot!(z6only, "v1__z6__{frm}-{to}");