rustrefactoringsimplify

How to simplify this Rust code with nested `match` with guards returning `Result`s?


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())),
        }
    }

Solution

  • 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;
    }