shepmaster / snafu

Easily assign underlying errors into domain-specific errors while adding context
https://docs.rs/snafu/
Apache License 2.0
1.45k stars 61 forks source link

Generated context selectors using public visibility cause failures in a crate with deny(missing_docs) #168

Closed Robzz closed 5 years ago

Robzz commented 5 years ago

I ran into a rough edge while porting some of my code to Snafu today. The crate has a !#[deny(missing_docs)] attribute and exports a snafu error type, I'd like to also export the generated context selectors so I'm using #[snafu(visibility = "pub")] as such:

#[derive(Debug, Snafu)]
#[snafu(visibility = "pub")]
/// ...
pub enum MyError {
    #[snafu(display(...))]
    /// HTTP error.
    Http { ... },
    #[snafu(display(...))]
    /// Network error.
    Network { ... },
    #[snafu(display(...))]
    /// Deserialization error.
    Serde { ... }
}

As the generated selectors don't have documentation, I get a compile error, and since the selector types are generated by the macro, I can't really add an #[allow(missing_docs)] annotation or add the documentation myself (at least, I couldn't find any documented way). The workaround I use is to define my snafu error type in a module with !#[allow(missing_docs)] at the top and pub use x::* from the parent module, turning the previous code into:

mod snafu_exports {
    #![allow(missing_docs)]

    #[derive(Debug, Snafu)]
    #[snafu(visibility = "pub")]
    /// ...
    pub enum MyError {
        #[snafu(display(...))]
        /// HTTP error.
        Http { ... },
        #[snafu(display(...))]
        /// Network error.
        Network { ... },
        #[snafu(display(...))]
        /// Deserialization error.
        Serde { ... }
}
pub use snafu_exports::*;

Which seems to work, but not having to use this workaround would be better. The simplest solutions I see are:

shepmaster commented 5 years ago
  • Generate some default documentation (something like "Snafu context selector for the X error variant" ?)

This seems like a low-effort enhancement that should be easy to do and worth it. I think that this documentation would be shown in IDEs and if someone decided to build the private crate docs.

  • Provide a way to write the documentation
  • Annotate/provide a way to annotate the generated code with #[allow(missing_docs)].

These would effectively be the same. Documentation comments become #[doc] attributes, so we could support something like:

enum Error {
    #[snafu(meta(doc = "Doc here"))]  // This
    #[snafu(meta(allow(missing_docs)))] // or this
    Variant,
}

but would that be very useful ?

If you are exporting the type, then presumably your users would find the documentation useful, yeah?

exports a snafu error type, I'd like to also export the generated context selectors

There's nothing intrinsically wrong with this, but it is surprising. Doing this will make the SNAFU-created code a part of your crate's public API; any future change to SNAFU might force you to release a semver-incompatible version of your crate.

Robzz commented 5 years ago

But would that be very useful ?

If you are exporting the type, then presumably your users would find the documentation useful, yeah?

Well, I meant, compared to default generated documentation, as in my first suggested resolution :) I personally don't have much more to say than "Hey, this is a Snafu context selector", but maybe someone else might, I don't know.

There's nothing intrinsically wrong with this, but it is surprising. Doing this will make the SNAFU-created code a part of your crate's public API; any future change to SNAFU might force you to release a semver-incompatible version of your crate.

That's fine by me, I'm doing this in a private personal project that only I will use (still, I prefer the thing well documented). But thank you for pointing this out, this is useful to keep in mind, and I'll try to avoid this pattern in any public crates.

I can work on this and submit a PR if you'd like, would you prefer the default documentation or a way to specify attributes ?

shepmaster commented 5 years ago

I can work on this and submit a PR if you'd like, would you prefer the default documentation or a way to specify attributes ?

That would be wonderful, thank you! Since

I think we should just go with the default generation.

As a note, you will probably also need to document the fields of the context selector as well.

Robzz commented 5 years ago

That works for me, I'll get on it in the next few days. Since #170 that you just opened is related, I can take care of it in the same PR if it's okay with you ?

shepmaster commented 5 years ago

Both parts sound great to me!