Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CLI Tool: Allow using --file - to read metadata from stdin #1336

Merged
merged 4 commits into from
Jan 3, 2024
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
116 changes: 98 additions & 18 deletions cli/src/utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ pub struct FileOrUrl {
pub url: Option<Url>,
/// The path to the encoded metadata file.
#[clap(long, value_parser)]
pub file: Option<PathBuf>,
pub file: Option<PathOrStdIn>,
/// Specify the metadata version.
///
/// - "latest": Use the latest stable version available.
Expand All @@ -33,6 +33,54 @@ pub struct FileOrUrl {
pub version: Option<MetadataVersion>,
}

impl FromStr for FileOrUrl {
type Err = &'static str;

fn from_str(s: &str) -> Result<Self, Self::Err> {
if let Ok(path) = PathOrStdIn::from_str(s) {
Ok(FileOrUrl {
url: None,
file: Some(path),
version: None,
})
} else {
Url::parse(s)
.map_err(|_| "Parsing Path or Uri failed.")
.map(|uri| FileOrUrl {
url: Some(uri),
file: None,
version: None,
})
}
}
}

/// If `--path -` is provided, read bytes for metadata from stdin
const STDIN_PATH_NAME: &str = "-";
#[derive(Debug, Clone)]
pub enum PathOrStdIn {
Path(PathBuf),
StdIn,
}

impl FromStr for PathOrStdIn {
type Err = &'static str;

fn from_str(s: &str) -> Result<Self, Self::Err> {
Copy link
Member

@niklasad1 niklasad1 Jan 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder whether we should trim the leading and trailing whitespaces before checking that the --file -

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and a simple test for the from_str impl would be nice to have.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think regarding trimming, clap already takes care of that before the args are given to us to parse, but I am not entirely sure.

Copy link
Collaborator

@jsdw jsdw Jan 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

worth a quick check but I'd assume the shell would handle it :)

(and if the user provides something like ' - ' as an argument then I wouldn't expect that to be treated as stdin anyways. Edit: Ah I can see it's trimmed now; that's fine too; either way works!)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tadeo added trimming and tests for it :)

let s = s.trim();
if s == STDIN_PATH_NAME {
Ok(PathOrStdIn::StdIn)
} else {
let path = std::path::Path::new(s);
if path.exists() {
Ok(PathOrStdIn::Path(PathBuf::from(path)))
} else {
Err("Path does not exist.")
}
}
}
}

impl FileOrUrl {
/// Fetch the metadata bytes.
pub async fn fetch(&self) -> color_eyre::Result<Vec<u8>> {
Expand All @@ -42,12 +90,20 @@ impl FileOrUrl {
eyre::bail!("specify one of `--url` or `--file` but not both")
}
// Load from --file path
(Some(path), None, None) => {
(Some(PathOrStdIn::Path(path)), None, None) => {
let mut file = fs::File::open(path)?;
let mut bytes = Vec::new();
file.read_to_end(&mut bytes)?;
Ok(bytes)
}
(Some(PathOrStdIn::StdIn), None, None) => {
let res = std::io::stdin().bytes().collect::<Result<Vec<u8>, _>>();

match res {
Ok(bytes) => Ok(bytes),
Err(err) => eyre::bail!("reading bytes from stdin (`--file -`) failed: {err}"),
}
}
// Cannot load the metadata from the file and specify a version to fetch.
(Some(_), None, Some(_)) => {
// Note: we could provide the ability to convert between metadata versions
Expand Down Expand Up @@ -88,25 +144,49 @@ pub fn with_indent(s: String, indent: usize) -> String {
.join("\n")
}

impl FromStr for FileOrUrl {
type Err = &'static str;
#[cfg(test)]
mod tests {
use crate::utils::{FileOrUrl, PathOrStdIn};
use std::str::FromStr;

fn from_str(s: &str) -> Result<Self, Self::Err> {
let path = std::path::Path::new(s);
if path.exists() {
#[test]
fn parsing() {
assert!(matches!(
FileOrUrl::from_str("-"),
Ok(FileOrUrl {
url: None,
file: Some(PathBuf::from(s)),
version: None,
file: Some(PathOrStdIn::StdIn),
version: None
})
} else {
Url::parse(s)
.map_err(|_| "no path or uri could be crated")
.map(|uri| FileOrUrl {
url: Some(uri),
file: None,
version: None,
})
}
),);

assert!(matches!(
FileOrUrl::from_str(" - "),
Ok(FileOrUrl {
url: None,
file: Some(PathOrStdIn::StdIn),
version: None
})
),);

assert!(matches!(
FileOrUrl::from_str("./src/main.rs"),
Ok(FileOrUrl {
url: None,
file: Some(PathOrStdIn::Path(_)),
version: None
})
),);

assert!(FileOrUrl::from_str("./src/i_dont_exist.rs").is_err());

assert!(matches!(
FileOrUrl::from_str("https://github.com/paritytech/subxt"),
Ok(FileOrUrl {
url: Some(_),
file: None,
version: None
})
));
}
}
Loading