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

Remove display/description boilerplate for simple error wrappers? #3

Open
untitaker opened this issue Oct 29, 2015 · 5 comments
Open

Comments

@untitaker
Copy link

Currently I have code that looks like this:

quick_error! {
    #[derive(Debug)]
    pub enum ServerError {
        Io(error: io::Error) {
            display("{}", error)
            description(error.description())
            cause(error)
            from()
        }
        JsonDecode(error: json::DecoderError) {
            display("{}", error)
            description(error.description())
            cause(error)
            from()
        }
        JsonEncode(error: json::EncoderError) {
            display("{}", error)
            description(error.description())
            cause(error)
            from()
        }
        ConfigError(error: config::Error) {
            display("{}", error)
            description(error.description())
            cause(error)
            from()
        }
    }
}

I see a few options to remove that boilerplate:

  • Make from() set those defaults automatically for simple wrappers.
  • Ability to override defaults on a per-enum basis, not only per-variant. No idea how syntax could look like.
@tailhook
Copy link
Owner

Probably it may look like:

quick_error! {
    #[derive(Debug)]
    pub enum ServerError {
        Io(error: io::Error) { wraps(error) }
    }
}

I.e. the important thing it that it should be easy to add additional variant that isn't a simple wrapper, but provides additional information. I'm not sure about good name for wraps. Any suggestions?

On the other hand, I usually use something like:

quick_error! {
    #[derive(Debug)]
    pub enum ServerError {
        Io(error: io::Error) {
            display("IO Error: {}", error)
            description("IO Error")
            cause(error)
            from()
        }
        JsonDecode(error: json::DecoderError) {
            display("Json decode error: {}", error)
            description("Json decode error")
            cause(error)
            from()
        }
    }
}

I'm not sure what the best practice is, but I have two reasons:

  1. I'm not sure that every error can unambiguously describe itself (i.e. what if config error can contain JsonDecode error itself, which means that parsing configuration failed)
  2. description() has higher level description (probably for the same reason as in No.1). If you wan't a description of contained error just call err.cause().description()

@untitaker
Copy link
Author

I see, but in my case I'd end up with console output like failed to launch server: failed to parse config: toml error: ... if I prefixed everything in my display impls. I agree about the impl of description though, yours is better.

Is there a way to let the user use their own macros inside the quick_error macro? That may solve this problem as well.

@canndrew
Copy link

I agree that using quick_error can involve a lot of unnecessary repetition. @tailhook's code above demonstrates this:

    JsonDecode(error: json::DecoderError) {
        display("Json decode error: {}", error)
        description("Json decode error")
        cause(error)
        from()
    }

Here, display repeats the entire body of description. It then manually prints out error even though we know that error is the cause of the error, that it implements Display and that it's probably safe to assume that the user wants to print it. This repetition will get even worse once this bug get's fixed. For example, here's what an error variant of mine might look like with doc-comments:

    /// Error connecting to a mapping server.
    MappingSocketConnect { addr: SocketAddr, err: io::Error } {
        description("Error connecting to a mapping server.")
        display("Error connecting to mapping server at address {}. Cause: {}", addr, err)
        cause(err)
    }

¡Ay, caramba! So much typing!

So here's some suggestions for improving this

  1. Once doc comments are enabled, description should default to the variant's doc comment.
  2. There should be another way to specify display such that it automatically prints out the description, followed by whatever format string and arguments you provide, followed by the cause of the error (if any). Perhaps this could be implemented by adding a new detail clause which causes display to default to this behaviour.

With the above, my error variant would now look like:

    /// Error connecting to a mapping server.
    MappingSocketConnect { addr: SocketAddr, err: io::Error } {
        detail("Mapping server address: {}.", addr)
        cause(err)
    }

Much more succinct

Which when displayed, would look something like Error connecting to a mapping server. Mapping server address: 1.2.3.4. Cause: Host unreachable

@tailhook
Copy link
Owner

Yeah. I was thinking about using docstrings too. The issue is that we don't have a good parser for docstrings. This means we can either use all the lines in the docstring or the first one.

Using all the lines from a docstring would probably lead to shortish doc comments without good description of the error.

And using just first line means you can't wrap it. I'm not sure this is a good thing either.

So, I'm kinda having mixed feelings for now.

@colin-kiegel
Copy link
Contributor

Assumption: Parsing of comments would probably mean additional recursions of the current macro.

Those recursions are limited by a setting of the compiler. The macro can only parse enums of certain length / complexity for any given recursion limit (without further trickery). This "ratio" would probably decline. With libmacro becoming stable this limitation will be gone. Until then, quick-error should be very conservative about using further recursions.

I would postpone this until stabilization of libmacro, if the assumption is true.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants