why HTTPError as return from HandlerFunc?

[tve]

Why did you make the breaking change to require HandlerFunc to return *HTTPError instead of error? Why not make HTTPError implement the Error() method and then it can be passed as error? This way a handler could either return a plain error or an HTTPError.

[vishr]

You can read more here http://blog.golang.org/error-handling-and-go. In short, I wanted to have a proper error type as convention instead of a generic error. The error object can still be set inside HTTPError struct and checked against in HTTPErrorHandler.

[tve]

Isn't the blog saying exactly what I'm saying, which is to leave error in the signature and pass a more specific type when appropriate? Being able to return err in handlers was one of the niceties of echo IMHO and now it becomes a cumbersome &echo.HttpError{Code:500, Message: ..., C: c} thing. If HTTPError implemented Error() then it could be passed as error but it wouldn't be required when a 500-error is all the user wants. Isn't this exactly what the blog post also says?

[vishr]

but it's the right thing to do here because ServeHTTP is the only place that sees the value and uses its contents.

The above statement holds true in this case as well. I am not against anything it's just that I wanted returning HTTPError to be the convention. You can even just return &HTTPError{}.

[tve]

Not sure what you're quoting. I guess I get to fork your repo then. What I proposed still would allow you to use HTTPError everywhere you want but doesn't force it. Too bad.

[vishr]

@tve It's from the same blog post. As caller of the handlers is ServeHTTP which should only be understanding HTTPError gives it more meaning. Also, project is already using *HTTPError from past couple of weeks and changing now will definitely break many people's code.

[raphael]

@vishr I think you're missing the point. From the article you are referring to:

A sophisticated caller can then use a type assertion to check for a NegativeSqrtError and handle it specially, while callers that just pass the error to fmt.Println or log.Fatal will see no change in behavior.

The handlers should return error, then user code can check for *HTTPError with an assertion and use that if needed. There's a lot of benefit deriving from using built-in types especially for error handling. For example every single implementation of an echo app is going to have to implement some Error() functiont that creates a *HTTPError from an error because that's what every other package use to return errors. I guess you could provide such a function but again that's not idiomatic.

[vishr]

@raphael If you look at the end of the post, the author mentions about returning appError in cases like this:

type appError struct {
    Error   error
    Message string
    Code    int
}

(It's usually a mistake to pass back the concrete type of an error rather than error, for reasons discussed in the Go FAQ, but it's the right thing to do here because ServeHTTP is the only place that sees the value and uses its contents.)

And make appHandler's ServeHTTP method display the appError's Message to the user with the correct HTTP status Code and log the full Error to the developer console:

With HTTPError in the signature handler signature it brings a good convention. For instance, if all handlers start sending any error (As they don't know about HTTPError) then the default centralized HTTP error handler won't be of much use, turning everything into 500.

Returning HTTPError is very easy with default error, just wrap it like this &HTTPError{Error: err}

Nothing is set in stone, I am open for discussion.

[raphael]

Thank you for the pointer, I understand the reference now. I guess I still don't see the benefit, it would be easy for echo to check whether the error is an HTTPError and if so use that to build the response otherwise call a generic error handler. The way it is now I have to wrap all errors in *HTTPError (like this), it would seem better if that was done in once place (the generic error handler). In general the code for the app before that change was cleaner.

[vishr]

How about if I add support for func (*echo.Context) error?

[raphael]

That would certainly help - I do wonder about how having two ways of defining handlers affects the usability of the framework though (you'd have to explain why there are two ways - when to use one vs. the other etc.). Probably something to ponder before making any change.

[vishr]

Fixed in #76.

[raphael]

Nice! thank you.