Skip to content
This repository has been archived by the owner on Apr 11, 2019. It is now read-only.

Ramp up test coverage #1

Merged
merged 1 commit into from
Nov 10, 2015
Merged

Ramp up test coverage #1

merged 1 commit into from
Nov 10, 2015

Conversation

tgulacsi
Copy link

@tgulacsi tgulacsi commented Nov 6, 2015

Not the real thing, mostly just to test everything is callable.

(I like your package, and want to use it, that's why I'm trying to polish it a little bit).

Not the real thing, mostly just to test everything is callable.
@tgulacsi
Copy link
Author

tgulacsi commented Nov 8, 2015

If the forced push hid my answer somewhere in the Abyss:

I've had a thought that this won't pass through :)
PTAL on the second attempt (forced update), this does hide the interfaces even more.

I wanted to export the interfaces just to be exact what kind of error should be passed to set the status/code.
Without this I've had to check the source how the returned status code is set on errors!

jjeffery added a commit that referenced this pull request Nov 10, 2015
@jjeffery jjeffery merged commit 4d989c6 into spkg:master Nov 10, 2015
@jjeffery
Copy link
Member

Thanks for the PR @tgulacsi. I'm not against making the interfaces public, and think that I will do so shortly. It's just that I have a few other packages that kind of work with this and they are not quite API stable yet. At the moment we are looking to go with

type Coder interface {
    Code() string
}

And

type Statuser() interface {
    Status() int
}

I'm not crazy about the names, but that seems to be the Go convention for interfaces with one method. What do you think?

@tgulacsi
Copy link
Author

I've no problem with Coder and Statuser, though net/http.Request uses StatusCode and Status, sticking to that'd have same pros, too.
But the main need is to export some interface, to allow consumers (users) to implement as they wish.

@jjeffery
Copy link
Member

Agreed that StatusCode might be better because of its use in the http.Response object. I'll look at using StatusCode instead of Status.

I'm interested in your comment that you need to export an interface to allow consumers to implement as they wish. I think one of the interesting things about Go is that a type can implement an interface without even knowing it exists. In the case of the httpctx package, if an error gets passed all the way back to the default error handling middleware, it is going to return a "500 Internal Server Error". It is just as a convenience that the default error handler will look for a StatusCode method.

The application is free to add whatever error handler they want to their middleware stack, and I think in most cases that would be a good idea. I don't think it is the job of the httpctx package to start exporting public interfaces to tell developers what kind of errors they should be returning. Do you know what I mean?

I appreciate your opinions Tamás. What say you open an issue if you think we should discuss this more.

In the case of the StatusCode methoc

@jjeffery
Copy link
Member

@tgulacsi Another thing FWIW is that I have uploaded another package that is kind of related to this one at https://github.com/spkg/slog -- this package is one we have used for logging in a few projects, and the interface is becoming stable. (I think we have to work on Status/StatusCode/Code though. There are a few other things that need work).

The point of this package is that it provides (yet another) logging package, and the interesting thing is that messages logged can also act as error instances. The messages also have a Status and a Code method, and these are the sorts of things that a custom httpctx middleware could pick up and use to return an application-specific error message.

@tgulacsi
Copy link
Author

@jjeffery you're absotely right that interfaces implemented implicitly in Go; I've tried to say that you must document the needed/used interfaces - now only the source code gives that information.

For such documentation, an exported interface is a nice solution - it is like a contract, even stronger than just documenting it on the function or in the package.

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

Successfully merging this pull request may close these issues.

2 participants