Skip to content

Commit cd8eb65

Browse files
MarijnS95MarijnS95
committed
Replace broken open-coding of FromStr with derive(clap::ValueEnum)
We've seen that `impl FromStr for Format` missed the `"exe"` match arm in parsing (#194) despite being listed in the allowed values in our top-level documentation and help text. Instead of fixing this mistake, remove open-coded `FromStr` entirely and rely on `clap`'s excellent `ValueEnum` derive. This not only generates a **complete** parser (with the option for attributes to change variant names) but also automates generation of a list of possible values, getting rid of any ambiguity/duplication/copy-paste errors we might have or create in the future: ❯ cargo r -- build -h ... Usage: x build [OPTIONS] Options: ... --platform <PLATFORM> Build artifacts for target platform [possible values: android, ios, linux, macos, windows] --arch <ARCH> Build artifacts for target arch [possible values: arm64, x64] ... --format <FORMAT> Build artifacts with format [possible values: aab, apk, appbundle, appdir, appimage, dmg, exe, ipa, msix] --store <STORE> Build artifacts for target app store [possible values: apple, microsoft, play, sideload] Finally, we also utilize its generated `PossibleValue::get_name()` to implement our open-coded `Display` implementations, ensuring any renames via clap attributes trickle through into how we print them.
1 parent 58eb125 commit cd8eb65

File tree

1 file changed

+25
-105
lines changed

1 file changed

+25
-105
lines changed

‎xbuild/src/lib.rs

+25-105
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ use crate::cargo::{Cargo, CargoBuild, CrateType};
22
use crate::config::Config;
33
use crate::devices::Device;
44
use anyhow::Result;
5-
use clap::Parser;
5+
use clap::{Parser, ValueEnum};
66
use std::path::{Path, PathBuf};
77
use xcommon::Signer;
88

@@ -40,7 +40,7 @@ impl std::fmt::Display for Opt {
4040
}
4141
}
4242

43-
#[derive(Clone, Copy, Debug, Eq, PartialEq)]
43+
#[derive(Clone, Copy, Debug, Eq, PartialEq, ValueEnum)]
4444
pub enum Platform {
4545
Android,
4646
Ios,
@@ -65,32 +65,14 @@ impl Platform {
6565

6666
impl std::fmt::Display for Platform {
6767
fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
68-
match self {
69-
Self::Android => write!(f, "android"),
70-
Self::Ios => write!(f, "ios"),
71-
Self::Linux => write!(f, "linux"),
72-
Self::Macos => write!(f, "macos"),
73-
Self::Windows => write!(f, "windows"),
74-
}
75-
}
76-
}
77-
78-
impl std::str::FromStr for Platform {
79-
type Err = anyhow::Error;
80-
81-
fn from_str(platform: &str) -> Result<Self> {
82-
Ok(match platform {
83-
"android" => Self::Android,
84-
"ios" => Self::Ios,
85-
"linux" => Self::Linux,
86-
"macos" => Self::Macos,
87-
"windows" => Self::Windows,
88-
_ => anyhow::bail!("unsupported platform {}", platform),
89-
})
68+
self.to_possible_value()
69+
.expect("No variant is skipped in clap")
70+
.get_name()
71+
.fmt(f)
9072
}
9173
}
9274

93-
#[derive(Clone, Copy, Debug, Eq, PartialEq)]
75+
#[derive(Clone, Copy, Debug, Eq, PartialEq, ValueEnum)]
9476
pub enum Arch {
9577
//Arm,
9678
Arm64,
@@ -112,30 +94,14 @@ impl Arch {
11294

11395
impl std::fmt::Display for Arch {
11496
fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
115-
match self {
116-
//Self::Arm => write!(f, "arm"),
117-
Self::Arm64 => write!(f, "arm64"),
118-
Self::X64 => write!(f, "x64"),
119-
//Self::X86 => write!(f, "x86"),
120-
}
121-
}
122-
}
123-
124-
impl std::str::FromStr for Arch {
125-
type Err = anyhow::Error;
126-
127-
fn from_str(arch: &str) -> Result<Self> {
128-
Ok(match arch {
129-
//"arm" => Self::Arm,
130-
"arm64" => Self::Arm64,
131-
"x64" => Self::X64,
132-
//"x86" => Self::X86,
133-
_ => anyhow::bail!("unsupported arch {}", arch),
134-
})
97+
self.to_possible_value()
98+
.expect("No variant is skipped in clap")
99+
.get_name()
100+
.fmt(f)
135101
}
136102
}
137103

138-
#[derive(Clone, Copy, Debug, Eq, PartialEq)]
104+
#[derive(Clone, Copy, Debug, Eq, PartialEq, ValueEnum)]
139105
pub enum Format {
140106
Aab,
141107
Apk,
@@ -150,35 +116,10 @@ pub enum Format {
150116

151117
impl std::fmt::Display for Format {
152118
fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
153-
match self {
154-
Self::Aab => write!(f, "aab"),
155-
Self::Apk => write!(f, "apk"),
156-
Self::Appbundle => write!(f, "appbundle"),
157-
Self::Appdir => write!(f, "appdir"),
158-
Self::Appimage => write!(f, "appimage"),
159-
Self::Dmg => write!(f, "dmg"),
160-
Self::Exe => write!(f, "exe"),
161-
Self::Ipa => write!(f, "ipa"),
162-
Self::Msix => write!(f, "msix"),
163-
}
164-
}
165-
}
166-
167-
impl std::str::FromStr for Format {
168-
type Err = anyhow::Error;
169-
170-
fn from_str(arch: &str) -> Result<Self> {
171-
Ok(match arch {
172-
"aab" => Self::Aab,
173-
"apk" => Self::Apk,
174-
"appbundle" => Self::Appbundle,
175-
"appdir" => Self::Appdir,
176-
"appimage" => Self::Appimage,
177-
"dmg" => Self::Dmg,
178-
"ipa" => Self::Ipa,
179-
"msix" => Self::Msix,
180-
_ => anyhow::bail!("unsupported arch {}", arch),
181-
})
119+
self.to_possible_value()
120+
.expect("No variant is skipped in clap")
121+
.get_name()
122+
.fmt(f)
182123
}
183124
}
184125

@@ -217,7 +158,7 @@ impl Format {
217158
}
218159
}
219160

220-
#[derive(Clone, Copy, Debug, Eq, PartialEq)]
161+
#[derive(Clone, Copy, Debug, Eq, PartialEq, ValueEnum)]
221162
pub enum Store {
222163
Apple,
223164
Microsoft,
@@ -227,26 +168,10 @@ pub enum Store {
227168

228169
impl std::fmt::Display for Store {
229170
fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
230-
match self {
231-
Self::Apple => write!(f, "apple"),
232-
Self::Microsoft => write!(f, "microsoft"),
233-
Self::Play => write!(f, "play"),
234-
Self::Sideload => write!(f, "sideload"),
235-
}
236-
}
237-
}
238-
239-
impl std::str::FromStr for Store {
240-
type Err = anyhow::Error;
241-
242-
fn from_str(store: &str) -> Result<Self> {
243-
Ok(match store {
244-
"apple" => Self::Apple,
245-
"microsoft" => Self::Microsoft,
246-
"play" => Self::Play,
247-
"sideload" => Self::Sideload,
248-
_ => anyhow::bail!("unsupported store {}", store),
249-
})
171+
self.to_possible_value()
172+
.expect("No variant is skipped in clap")
173+
.get_name()
174+
.fmt(f)
250175
}
251176
}
252177

@@ -376,25 +301,20 @@ pub struct BuildTargetArgs {
376301
/// Build artifacts in release mode, with optimizations
377302
#[clap(long, short, conflicts_with = "debug")]
378303
release: bool,
379-
/// Build artifacts for target platform. Can be one of
380-
/// `android`, `ios`, `linux`, `macos` or `windows`.
304+
/// Build artifacts for target platform.
381305
#[clap(long, conflicts_with = "device")]
382306
platform: Option<Platform>,
383-
/// Build artifacts for target arch. Can be one of
384-
/// `arm64` or `x64`.
307+
/// Build artifacts for target arch.
385308
#[clap(long, requires = "platform")]
386309
arch: Option<Arch>,
387310
/// Build artifacts for target device. To find the device
388311
/// identifier of a connected device run `x devices`.
389312
#[clap(long, conflicts_with = "store")]
390313
device: Option<String>,
391-
/// Build artifacts with format. Can be one of `aab`,
392-
/// `apk`, `appbundle`, `appdir`, `appimage`, `dmg`,
393-
/// `exe`, `ipa`, `msix`.
314+
/// Build artifacts with format.
394315
#[clap(long, conflicts_with = "store")]
395316
format: Option<Format>,
396-
/// Build artifacts for target app store. Can be one of
397-
/// `apple`, `microsoft`, `play` or `sideload`.
317+
/// Build artifacts for target app store.
398318
#[clap(long, conflicts_with = "device", conflicts_with = "format")]
399319
store: Option<Store>,
400320
/// Path to a PEM encoded RSA2048 signing key and certificate

0 commit comments

Comments
 (0)