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

Change the default context selector suffix to Snafu #286

Merged
merged 4 commits into from Jul 5, 2021
Merged

Conversation

shepmaster
Copy link
Owner

No description provided.

@shepmaster shepmaster changed the title Support stringly typed errors Change the default context selector suffix to Snafu May 3, 2021
@shepmaster shepmaster added breaking change Likely requires a SemVer version bump enhancement New feature or request maintenance Keeping the wheels turning labels May 3, 2021
@shepmaster shepmaster force-pushed the snafu-suffix branch 4 times, most recently from dd06ca4 to ea0a324 Compare May 5, 2021 01:56
@SamWilsn
Copy link
Contributor

SamWilsn commented Jul 2, 2021

Hopefully dropping a comment here isn't too forward ^^;;

I routinely use a pattern like this with snafu:

src/error.rs

pub(crate) mod parse_error {
    use snauf::Snafu;

    #[derive(Debug, Snafu)]
    #[snafu(visibility = "pub(crate)")]
    pub enum ParseError {
        SomeBadThing {
            source: SomeOtherError,
        }
    }
}

pub use self::parse_error::ParseError;

src/main.rs

pub mod errors;

use self::errors::{parse_error, ParseError};

use snafu::ResultExt;

fn main() -> Result<(), ParseError> {
    do_something()
        .context(parse_error::SomeBadThing)?;

    Ok(())
}

tl;dr I think a #[snafu(module = "parse_error")] option might be better than a suffix (though I'm not super familiar with macro hygiene, so it might not be possible at all.)

@shepmaster
Copy link
Owner Author

Hopefully dropping a comment here isn't too forward ^^;;

Not at all!

a #[snafu(module = "parse_error")] option

Presumably this would create the module and then place the error and selectors into it, similar to what is described in #188? Ultimately, I don't think that approach can work.

Note that your example uses SomeOtherError. That's never imported in the module, so it'd generate a compiler error (perhaps this is what you meant by hygiene?). Even if it was imported elsewhere, the macro only knows about the literal text that the derive is attached to, so we'd only see SomeOtherError, not the path it was imported from.

At best, you'd have to provide fully-qualified paths for many of the variant fields.

@SamWilsn
Copy link
Contributor

SamWilsn commented Jul 2, 2021

Presumably this would create the module and then place the error and selectors into it, similar to what is described in #188?

Oh, yes. That's pretty much it exactly.

Note that your example uses SomeOtherError. That's never imported in the module, so it'd generate a compiler error

Right. I would've needed to use super::*; for this to work.

perhaps this is what you meant by hygiene?

I was speaking more about the restrictions on what identifiers escape a macro. I think there are some rules on it, and I'm absolutely not familiar with them.

Even if it was imported elsewhere, the macro only knows about the literal text that the derive is attached to, so we'd only see SomeOtherError, not the path it was imported from.

At best, you'd have to provide fully-qualified paths for many of the variant fields.

Would you be able to expand the derive macro to a module with either:

  1. use super::{<types>}; where <types> is a comma delimited list of the error types from the enum, or
  2. use super::*;?

I doubt (1) would work with :: in types (crate::FooError or ::other_crate::BarError), but I think (2) would usually work?

So:

use dependency::SomeOtherError;

#[derive(Debug, Snafu)]
#[snafu(module)] // Let's pretend that `module` does a camel to snake-case conversion, and `module(...)` lets you name it?
pub enum ParseError {
    SomeOther { source: SomeOtherError },
}

Would expand to something like:

use dependency::SomeOtherError;

#[derive(Debug)]
pub enum ParseError {
    // ...
}

pub(crate) mod parse_error {
    use super::*;

    pub(crate) SomeOther {
        // ...
    }
}

@shepmaster
Copy link
Owner Author

I think [use super::*;] would usually work?

I can see that working in many cases, but it'd probably fall down for some other cases:

#[derive(Snafu,Debug)]
pub enum ParseError {
    Thing { source: super::Error },
}

be better than a suffix

I'm not a fan of the module-focused view. My personal direction is that each module should have it's own error type and selectors. The selectors should only be used in that module.

That being said, it seems that both solutions can coexist. Would you be interested in (a) taking this discussion to #188 and/or (b) attempting to implement it?

@shepmaster shepmaster marked this pull request as ready for review July 5, 2021 13:45
@shepmaster shepmaster merged commit 43257dd into master Jul 5, 2021
@shepmaster shepmaster deleted the snafu-suffix branch July 5, 2021 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Likely requires a SemVer version bump enhancement New feature or request maintenance Keeping the wheels turning
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants