Skip to content

Commit d9da872

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 be5b43f commit d9da872

File tree

1 file changed

+25
-106
lines changed

1 file changed

+25
-106
lines changed

‎xbuild/src/lib.rs

+25-106
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,36 +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(format: &str) -> Result<Self> {
171-
Ok(match format {
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-
"exe" => Self::Exe,
179-
"ipa" => Self::Ipa,
180-
"msix" => Self::Msix,
181-
_ => anyhow::bail!("unsupported format {}", format),
182-
})
119+
self.to_possible_value()
120+
.expect("No variant is skipped in clap")
121+
.get_name()
122+
.fmt(f)
183123
}
184124
}
185125

@@ -218,7 +158,7 @@ impl Format {
218158
}
219159
}
220160

221-
#[derive(Clone, Copy, Debug, Eq, PartialEq)]
161+
#[derive(Clone, Copy, Debug, Eq, PartialEq, ValueEnum)]
222162
pub enum Store {
223163
Apple,
224164
Microsoft,
@@ -228,26 +168,10 @@ pub enum Store {
228168

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

@@ -377,25 +301,20 @@ pub struct BuildTargetArgs {
377301
/// Build artifacts in release mode, with optimizations
378302
#[clap(long, short, conflicts_with = "debug")]
379303
release: bool,
380-
/// Build artifacts for target platform. Can be one of
381-
/// `android`, `ios`, `linux`, `macos` or `windows`.
304+
/// Build artifacts for target platform.
382305
#[clap(long, conflicts_with = "device")]
383306
platform: Option<Platform>,
384-
/// Build artifacts for target arch. Can be one of
385-
/// `arm64` or `x64`.
307+
/// Build artifacts for target arch.
386308
#[clap(long, requires = "platform")]
387309
arch: Option<Arch>,
388310
/// Build artifacts for target device. To find the device
389311
/// identifier of a connected device run `x devices`.
390312
#[clap(long, conflicts_with = "store")]
391313
device: Option<String>,
392-
/// Build artifacts with format. Can be one of `aab`,
393-
/// `apk`, `appbundle`, `appdir`, `appimage`, `dmg`,
394-
/// `exe`, `ipa`, `msix`.
314+
/// Build artifacts with format.
395315
#[clap(long, conflicts_with = "store")]
396316
format: Option<Format>,
397-
/// Build artifacts for target app store. Can be one of
398-
/// `apple`, `microsoft`, `play` or `sideload`.
317+
/// Build artifacts for target app store.
399318
#[clap(long, conflicts_with = "device", conflicts_with = "format")]
400319
store: Option<Store>,
401320
/// Path to a PEM encoded RSA2048 signing key and certificate

0 commit comments

Comments
 (0)