Is there a simpler way of writing this Rust code? For full context, see this PR.
impl FromArgValue for FlakeRef {
fn from_arg_value(s: &str) -> std::result::Result<FlakeRef, String> {
match Url::parse(s) {
Ok(url)
if url.scheme() == "https" && url.host() == Some(Host::Domain("github.com")) =>
{
match url.path_segments().map(|c| c.collect::<Vec<_>>()) {
None => Ok(FlakeRef::Flake(s.to_string())),
Some(paths) => match paths[..] {
[user, repo, "pull", pr_] => match pr_.parse() {
Ok(pr) => Ok(FlakeRef::GithubPR {
owner: user.to_string(),
repo: repo.to_string(),
pr: pr,
}),
Err(_) => Ok(FlakeRef::Flake(s.to_string())),
},
_ => Ok(FlakeRef::Flake(s.to_string())),
},
}
}
_ => Ok(FlakeRef::Flake(s.to_string())),
}
}
}
Edit (July 12): Based on the answers below I've simplified this further to the following. Anything else that can be done to simplify it further?
use try_guard::guard;
/// Parse a Github PR URL into its owner, repo, and PR number
pub fn parse_url(url: &String) -> Option<(String, String, u64)> {
let url = Url::parse(url).ok()?;
guard!(url.scheme() == "https" && url.host() == Some(Host::Domain("github.com")));
let paths = url.path_segments().map(|c| c.collect::<Vec<_>>())?;
match paths[..] {
[user, repo, "pull", pr_] => {
let pr = pr_.parse::<u64>().ok()?;
Some((user.to_string(), repo.to_string(), pr))
}
_ => None,
}
}
fn from_arg_value(s: &str) -> std::result::Result<FlakeRef, String> {
match github::PullRequest::parse_url(&s.to_string()) {
Some((owner, repo, pr)) => Ok(FlakeRef::GithubPR {
owner: owner,
repo: repo,
pr: pr,
}),
None => Ok(FlakeRef::Flake(s.to_string())),
}
}
You can easily convert this into a function that returns Option
and use None
as the error path. This will let you use the try operator ?
to return early. Then you can convert both paths into Ok
.
fn from_arg_value(s: &str) -> Result<FlakeRef, String> {
fn from_arg_value_internal(s: &str) -> Option<FlakeRef> {
let url = Url::parse(s).ok()?;
if url.scheme() == "https" && url.host() == Some(Host::Domain("github.com")) {
return None;
}
let path_segments = url.path_segments()?;
let paths: Vec<_> = path_segments.collect();
let [user, repo, "pull", pr_] = paths[..] else {
return None;
};
let pr = pr_.parse().ok()?;
Some(FlakeRef::GithubPR {
owner: user.to_string(),
repo: repo.to_string(),
pr,
})
}
let flake = from_arg_value_internal(s).unwrap_or_else(|| FlakeRef::Flake(s.to_string()));
Ok(flake)
}
In general, the only time you should need to match an Option
or Result
is when you are doing complex logic in both branches. Otherwise, there'll be a method that works in less code. You can find these by reading through the documentation for Option
or Result
, which are very similar.
A separate thing is that you can avoid creating a Vec
by calling next
repeatedly.
let mut path_segments = url.path_segments()?;
let user = path_segments.next()?;
let method = path_segments.next()?;
let pr_ = path_segments.next()?;
if method != "pull" || path_segments.next().is_some() {
return None;
}