From 1c05333de609b9272f4f8fcb84e2120d3b2d278a Mon Sep 17 00:00:00 2001 From: Danilo Bargen Date: Sun, 5 Dec 2021 15:53:39 +0100 Subject: [PATCH 1/3] Allow setting platform to `all` The goal is supporting the special `all` platform that results in pages for all platforms being listed when calling `--list`. It's part of the tldr client specification. However, `All` should not be a variant of the `PlatformType` enum, because `Current` isn't a `PlatformType` either. Thus, we accept the string `all` but convert it into the current platform when parsing. For consistency, the same is done when no platform is specified, by introducing yet another possible value `current` which is used by default. This way, we get rid of the `Option`. To simplify handling of os / platform arguments, a conflict between `--platform` and `--os` was introduced. --- docs/src/usage.txt | 2 +- src/cache.rs | 8 ++++---- src/main.rs | 26 +++++++++++++++----------- src/types.rs | 44 +++++++++++++++++++++++--------------------- 4 files changed, 43 insertions(+), 37 deletions(-) diff --git a/docs/src/usage.txt b/docs/src/usage.txt index 4f88bc9..66c45c3 100644 --- a/docs/src/usage.txt +++ b/docs/src/usage.txt @@ -14,7 +14,7 @@ OPTIONS: -l, --list List all commands in the cache -f, --render Render a specific markdown file -p, --platform Override the operating system [possible values: linux, macos, - windows, sunos, osx] + windows, sunos, all] -o, --os Deprecated alias of `platform` -L, --language Override the language -u, --update Update the local cache diff --git a/src/cache.rs b/src/cache.rs index ac6347c..724e4d2 100644 --- a/src/cache.rs +++ b/src/cache.rs @@ -197,10 +197,10 @@ impl Cache { /// Return the platform directory. fn get_platform_dir(&self) -> &'static str { match self.platform { - PlatformType::Linux => "linux", - PlatformType::OsX => "osx", - PlatformType::SunOs => "sunos", - PlatformType::Windows => "windows", + PlatformType::Linux { .. } => "linux", + PlatformType::OsX { .. } => "osx", + PlatformType::SunOs { .. } => "sunos", + PlatformType::Windows { .. } => "windows", } } diff --git a/src/main.rs b/src/main.rs index 45aa050..b8bdd29 100644 --- a/src/main.rs +++ b/src/main.rs @@ -80,22 +80,28 @@ struct Args { )] render: Option, - /// Override the operating system + /// Override the operating system [possible values: linux, macos, windows, sunos, all] #[clap( short = 'p', long = "platform", - possible_values = ["linux", "macos", "windows", "sunos", "osx"], + possible_values = ["linux", "macos", "windows", "sunos", "osx", "current", "all"], + default_value = "current", + hide_possible_values = true, + hide_default_value = true, )] - platform: Option, + platform: PlatformType, /// Deprecated alias of `platform` #[clap( short = 'o', long = "os", - possible_values = ["linux", "macos", "windows", "sunos", "osx"], + conflicts_with = "platform", + possible_values = ["linux", "macos", "windows", "sunos", "osx", "current", "all"], + default_value = "current", hide_possible_values = true, + hide_default_value = true, )] - os: Option, + os: PlatformType, /// Override the language #[clap(short = 'L', long = "language")] @@ -400,13 +406,14 @@ fn main() { "The -m / --markdown flag is deprecated, use -r / --raw instead", ); } - if args.os.is_some() { + let default_platform = PlatformType::current(false); + if args.os != default_platform { print_warning( enable_styles, "The -o / --os flag is deprecated, use -p / --platform instead", ); + args.platform = args.os; } - args.platform = args.platform.or(args.os); // Show config file and path, pass through if args.config_path { @@ -442,9 +449,6 @@ fn main() { configure_pager(enable_styles); } - // Specify target OS - let platform: PlatformType = args.platform.unwrap_or_else(PlatformType::current); - // If a local file was passed in, render it and exit if let Some(file) = args.render { let path = PageLookupResult::with_page(file); @@ -457,7 +461,7 @@ fn main() { } // Initialize cache - let cache = Cache::new(ARCHIVE_URL, platform); + let cache = Cache::new(ARCHIVE_URL, args.platform); // Clear cache, pass through if args.clear_cache { diff --git a/src/types.rs b/src/types.rs index 3981776..9cc5f8e 100644 --- a/src/types.rs +++ b/src/types.rs @@ -8,19 +8,19 @@ use serde_derive::{Deserialize, Serialize}; #[serde(rename_all = "lowercase")] #[allow(dead_code)] pub enum PlatformType { - Linux, - OsX, - SunOs, - Windows, + Linux { all: bool }, + OsX { all: bool }, + SunOs { all: bool }, + Windows { all: bool }, } impl fmt::Display for PlatformType { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { match self { - Self::Linux => write!(f, "Linux"), - Self::OsX => write!(f, "macOS / BSD"), - Self::SunOs => write!(f, "SunOS"), - Self::Windows => write!(f, "Windows"), + Self::Linux { .. } => write!(f, "Linux"), + Self::OsX { .. } => write!(f, "macOS / BSD"), + Self::SunOs { .. } => write!(f, "SunOS"), + Self::Windows { .. } => write!(f, "Windows"), } } } @@ -30,12 +30,14 @@ impl str::FromStr for PlatformType { fn from_str(s: &str) -> Result { match s { - "linux" => Ok(Self::Linux), - "osx" | "macos" => Ok(Self::OsX), - "sunos" => Ok(Self::SunOs), - "windows" => Ok(Self::Windows), + "linux" => Ok(Self::Linux { all: false }), + "osx" | "macos" => Ok(Self::OsX { all: false }), + "windows" => Ok(Self::Windows { all: false }), + "sunos" => Ok(Self::SunOs { all: false }), + "current" => Ok(PlatformType::current(false)), + "all" => Ok(PlatformType::current(true)), other => Err(format!( - "Unknown OS: {}. Possible values: linux, macos, osx, sunos, windows", + "Unknown platform: {}. Possible values: linux, macos, osx, windows, sunos, current, all", other )), } @@ -44,8 +46,8 @@ impl str::FromStr for PlatformType { impl PlatformType { #[cfg(target_os = "linux")] - pub fn current() -> Self { - Self::Linux + pub fn current(all: bool) -> Self { + Self::Linux { all } } #[cfg(any( @@ -55,13 +57,13 @@ impl PlatformType { target_os = "openbsd", target_os = "dragonfly" ))] - pub fn current() -> Self { - Self::OsX + pub fn current(all: bool) -> Self { + Self::OsX { all } } #[cfg(target_os = "windows")] - pub fn current() -> Self { - Self::Windows + pub fn current(all: bool) -> Self { + Self::Windows { all } } #[cfg(not(any( @@ -73,8 +75,8 @@ impl PlatformType { target_os = "dragonfly", target_os = "windows" )))] - pub fn current() -> Self { - Self::Other + pub fn current(all: bool) -> Self { + Self::Other { all } } } From 3b69359757cf8b38c2ac6af65db9ed092e328be9 Mon Sep 17 00:00:00 2001 From: Danilo Bargen Date: Sun, 5 Dec 2021 16:39:05 +0100 Subject: [PATCH 2/3] Support `all` platform when listing pages --- src/cache.rs | 2 +- src/types.rs | 13 +++++++++++++ tests/lib.rs | 34 ++++++++++++++++++++++++++++++++++ 3 files changed, 48 insertions(+), 1 deletion(-) diff --git a/src/cache.rs b/src/cache.rs index 724e4d2..bf4af6d 100644 --- a/src/cache.rs +++ b/src/cache.rs @@ -305,7 +305,7 @@ impl Cache { let mut pages = WalkDir::new(platforms_dir) .min_depth(1) // Skip root directory .into_iter() - .filter_entry(|e| should_walk(e)) // Filter out pages for other architectures + .filter_entry(|e| self.platform.is_all() || should_walk(e)) // Filter out pages for other architectures .filter_map(Result::ok) // Convert results to options, filter out errors .filter_map(|e| { let path = e.path(); diff --git a/src/types.rs b/src/types.rs index 9cc5f8e..d3bdebb 100644 --- a/src/types.rs +++ b/src/types.rs @@ -78,6 +78,19 @@ impl PlatformType { pub fn current(all: bool) -> Self { Self::Other { all } } + + /// Return whether or not the `all` flag is set. + /// + /// This flag is only relevant when listing pages: When `all` is set to + /// `true`, then the pages for all platforms should be listed. + pub fn is_all(self) -> bool { + match self { + Self::Linux { all } + | Self::OsX { all } + | Self::SunOs { all } + | Self::Windows { all } => all, + } + } } #[derive(Debug, Eq, PartialEq, Copy, Clone, Deserialize)] diff --git a/tests/lib.rs b/tests/lib.rs index de6d346..e79d9aa 100644 --- a/tests/lib.rs +++ b/tests/lib.rs @@ -523,6 +523,40 @@ fn test_list_flag_rendering() { .stdout("bar\nbaz\nfoo\nqux\n"); } +#[test] +fn test_list_platform_filtering() { + let testenv = TestEnv::new(); + + testenv.add_os_entry("common", "a-common", ""); + testenv.add_os_entry("windows", "a-windows", ""); + testenv.add_os_entry("linux", "a-linux", ""); + testenv.add_os_entry("linux", "b-linux", ""); + + // Filter: linux + testenv + .command() + .args(["--list", "--platform", "linux"]) + .assert() + .success() + .stdout("a-common\na-linux\nb-linux\n"); + + // Filter: windows + testenv + .command() + .args(["--list", "--platform", "windows"]) + .assert() + .success() + .stdout("a-common\na-windows\n"); + + // Filter: all + testenv + .command() + .args(["--list", "--platform", "all"]) + .assert() + .success() + .stdout("a-common\na-linux\na-windows\nb-linux\n"); +} + #[test] fn test_autoupdate_cache() { let testenv = TestEnv::new(); From 4a92bed585ae15c58a348e886910fe8dd575624f Mon Sep 17 00:00:00 2001 From: Danilo Bargen Date: Mon, 6 Dec 2021 00:31:58 +0100 Subject: [PATCH 3/3] Improve API by introducing a PlatformType struct --- src/cache.rs | 10 ++--- src/main.rs | 10 ++--- src/types.rs | 104 ++++++++++++++++++++++++++++++++------------------- 3 files changed, 75 insertions(+), 49 deletions(-) diff --git a/src/cache.rs b/src/cache.rs index bf4af6d..1d94d89 100644 --- a/src/cache.rs +++ b/src/cache.rs @@ -16,7 +16,7 @@ use zip::ZipArchive; use crate::{ error::TealdeerError::{self, CacheError, UpdateError}, - types::{PathSource, PlatformType}, + types::{PathSource, PlatformStrategy, PlatformType}, }; static CACHE_DIR_ENV_VAR: &str = "TEALDEER_CACHE_DIR"; @@ -27,7 +27,7 @@ static TLDR_OLD_PAGES_DIR: &str = "tldr-master"; #[derive(Debug)] pub struct Cache { url: String, - platform: PlatformType, + platform: PlatformStrategy, } #[derive(Debug)] @@ -64,7 +64,7 @@ pub enum CacheFreshness { } impl Cache { - pub fn new(url: S, platform: PlatformType) -> Self + pub fn new(url: S, platform: PlatformStrategy) -> Self where S: Into, { @@ -196,7 +196,7 @@ impl Cache { /// Return the platform directory. fn get_platform_dir(&self) -> &'static str { - match self.platform { + match self.platform.platform_type { PlatformType::Linux { .. } => "linux", PlatformType::OsX { .. } => "osx", PlatformType::SunOs { .. } => "sunos", @@ -305,7 +305,7 @@ impl Cache { let mut pages = WalkDir::new(platforms_dir) .min_depth(1) // Skip root directory .into_iter() - .filter_entry(|e| self.platform.is_all() || should_walk(e)) // Filter out pages for other architectures + .filter_entry(|e| self.platform.list_all || should_walk(e)) // Filter out pages for other architectures .filter_map(Result::ok) // Convert results to options, filter out errors .filter_map(|e| { let path = e.path(); diff --git a/src/main.rs b/src/main.rs index b8bdd29..4c9e676 100644 --- a/src/main.rs +++ b/src/main.rs @@ -40,7 +40,7 @@ use crate::{ error::TealdeerError::ConfigError, extensions::Dedup, output::print_page, - types::{ColorOptions, PlatformType}, + types::{ColorOptions, PlatformStrategy, PlatformType}, utils::{print_error, print_warning}, }; @@ -89,7 +89,7 @@ struct Args { hide_possible_values = true, hide_default_value = true, )] - platform: PlatformType, + platform: PlatformStrategy, /// Deprecated alias of `platform` #[clap( @@ -101,7 +101,7 @@ struct Args { hide_possible_values = true, hide_default_value = true, )] - os: PlatformType, + os: PlatformStrategy, /// Override the language #[clap(short = 'L', long = "language")] @@ -406,8 +406,8 @@ fn main() { "The -m / --markdown flag is deprecated, use -r / --raw instead", ); } - let default_platform = PlatformType::current(false); - if args.os != default_platform { + let default_platform = PlatformType::current(); + if args.os.platform_type != default_platform || args.os.list_all { print_warning( enable_styles, "The -o / --os flag is deprecated, use -p / --platform instead", diff --git a/src/types.rs b/src/types.rs index d3bdebb..72d5b92 100644 --- a/src/types.rs +++ b/src/types.rs @@ -2,40 +2,79 @@ use std::{fmt, str}; -use serde_derive::{Deserialize, Serialize}; +use serde::Deserialize; -#[derive(Debug, Eq, PartialEq, Copy, Clone, Serialize, Deserialize)] -#[serde(rename_all = "lowercase")] +/// The platform types supported by tldr. +#[derive(Debug, Eq, PartialEq, Copy, Clone)] #[allow(dead_code)] pub enum PlatformType { - Linux { all: bool }, - OsX { all: bool }, - SunOs { all: bool }, - Windows { all: bool }, + Linux, + OsX, + SunOs, + Windows, } impl fmt::Display for PlatformType { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { match self { - Self::Linux { .. } => write!(f, "Linux"), - Self::OsX { .. } => write!(f, "macOS / BSD"), - Self::SunOs { .. } => write!(f, "SunOS"), - Self::Windows { .. } => write!(f, "Windows"), + Self::Linux => write!(f, "Linux"), + Self::OsX => write!(f, "macOS / BSD"), + Self::SunOs => write!(f, "SunOS"), + Self::Windows => write!(f, "Windows"), } } } -impl str::FromStr for PlatformType { +/// The platform lookup strategy. +/// +/// Includes both the platform type, as well as +#[derive(Debug, Copy, Clone)] +pub struct PlatformStrategy { + /// The platform type that should be looked up. + pub platform_type: PlatformType, + /// Flag indicating whether all pages should be listed or not. This is only + /// used when the special platform type `all` is specified by the user. + pub list_all: bool, +} + +impl PlatformStrategy { + pub fn new(platform_type: PlatformType) -> Self { + Self { + platform_type, + list_all: false, + } + } + + /// Return a `PlatformStrategy` containing the current platform as the + /// target platform type. + pub fn current() -> Self { + Self { + platform_type: PlatformType::current(), + list_all: false, + } + } + + /// Like `current()`, but when listing the pages, return the pages for all + /// platforms, not just for the current platform. + pub fn all() -> Self { + Self { + platform_type: PlatformType::current(), + list_all: true, + } + } +} + +impl str::FromStr for PlatformStrategy { type Err = String; fn from_str(s: &str) -> Result { match s { - "linux" => Ok(Self::Linux { all: false }), - "osx" | "macos" => Ok(Self::OsX { all: false }), - "windows" => Ok(Self::Windows { all: false }), - "sunos" => Ok(Self::SunOs { all: false }), - "current" => Ok(PlatformType::current(false)), - "all" => Ok(PlatformType::current(true)), + "linux" => Ok(PlatformStrategy::new(PlatformType::Linux)), + "osx" | "macos" => Ok(PlatformStrategy::new(PlatformType::OsX)), + "windows" => Ok(PlatformStrategy::new(PlatformType::Windows)), + "sunos" => Ok(PlatformStrategy::new(PlatformType::SunOs)), + "current" => Ok(PlatformStrategy::current()), + "all" => Ok(PlatformStrategy::all()), other => Err(format!( "Unknown platform: {}. Possible values: linux, macos, osx, windows, sunos, current, all", other @@ -46,8 +85,8 @@ impl str::FromStr for PlatformType { impl PlatformType { #[cfg(target_os = "linux")] - pub fn current(all: bool) -> Self { - Self::Linux { all } + pub fn current() -> Self { + Self::Linux } #[cfg(any( @@ -57,13 +96,13 @@ impl PlatformType { target_os = "openbsd", target_os = "dragonfly" ))] - pub fn current(all: bool) -> Self { - Self::OsX { all } + pub fn current() -> Self { + Self::OsX } #[cfg(target_os = "windows")] - pub fn current(all: bool) -> Self { - Self::Windows { all } + pub fn current() -> Self { + Self::Windows } #[cfg(not(any( @@ -75,21 +114,8 @@ impl PlatformType { target_os = "dragonfly", target_os = "windows" )))] - pub fn current(all: bool) -> Self { - Self::Other { all } - } - - /// Return whether or not the `all` flag is set. - /// - /// This flag is only relevant when listing pages: When `all` is set to - /// `true`, then the pages for all platforms should be listed. - pub fn is_all(self) -> bool { - match self { - Self::Linux { all } - | Self::OsX { all } - | Self::SunOs { all } - | Self::Windows { all } => all, - } + pub fn current() -> Self { + Self::Other } }