API of public struct

#[derive(Debug, Clone, Hash, Default, PartialEq, Eq)]
pub struct NZB {
    pub meta: Meta,
    pub files: Vec<File>,
}

impl NZB {
    pub fn new(meta: Meta, files: Vec<File>) -> Self {
        Self { meta, files }
    }

    pub fn parse(xml: impl AsRef<str>) -> Result<Self, InvalidNZBError> {
        let xml = xml.as_ref();
        let xml = sanitize_xml(xml);
        let nzb = match Document::parse(xml) {
            Ok(doc) => doc,
            Err(err) => return Err(InvalidNZBError::new(err.to_string())),
        };
        let meta = parse_metadata(&nzb);
        let files = match parse_files(&nzb) {
            Ok(files) => files,
            Err(err) => return Err(InvalidNZBError::new(err.to_string())),
        };
        Ok(NZB::new(meta, files))
    }
}

I'm not really sure what's the idiomatic rust approach to writing an API like this. In this case fn new is essentially useless, since a user would always supply the xml string itself. My options seem to be:

  1. Rename fn parse to fn new, remove fn parse.
  2. Keep it as is.
  3. Provide a top level pub fn parse outside of the impl NZB? e.g.
    use mylib::parse;
    let nzb = parse(xml);
    

I'd keep it as is or replace parse with a FromStr implementation. I usually don't have non-trivial new constructors on my structs that are fallible and I don't think structs without a new associated function are weird. Especially when all fields are public and the struct isn't marked as #[non_exhaustive].

4 Likes

That makes a lot of sense. I wasn't aware of the FromStr trait either, thank you!

1 Like

I don't think this line of thought is actually convincing though. It's entirely possible that someone would generate the contents in some explicit programmatic way, not via parsing XML. The fact that all fields are public, however, is actually an argument against fn new, here I agree with jofas.

1 Like

Something I just realized, I can't use NZB::from_str without use std::str::FromStr;, is there a way around it?

The more common way to use FromStr::from_str is via str::parse().

2 Likes