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

Break package into client and router #93

Open
jcelliott opened this issue Sep 2, 2015 · 4 comments
Open

Break package into client and router #93

jcelliott opened this issue Sep 2, 2015 · 4 comments
Labels

Comments

@jcelliott
Copy link
Owner

It seems unfortunate that in order to use Turnpike's client API you have to import the entire router implementation. It seems like splitting the package would make things a bit cleaner. I propose to change the package structure to the following:

turnpike/
    client.go
    internal/
        message.go
        ...
    common/
        # not sure, anything common to client and router code that also should be exported
    router/
        router.go
        ...

With this structure, client would still be imported as .../turnpike, and router code would be imported as .../turnpike/router. It would use the Go 1.4 internal packages feature to prevent exporting unnecessary code.

I would like to get turnpike v2 to a stable release, and I think this would help move in that direction. @beatgammit @marshauf @DaniloMoura1 @thephred +anyone else: any suggestions or objections?

@jcelliott
Copy link
Owner Author

Related to this, I would like to un-export anything that doesn't need to be exported. I don't think the WAMP predefined error URIs (util.go) should be exported, as well as all the WAMP message types (message.go). Unless anyone needs those to be exported, I'm going to move them. It really clutters the package API (see the godoc result for turnpike).

@marshauf
Copy link
Collaborator

marshauf commented Sep 3, 2015

The net/http package has server and client related code in it for example.
I used WAMP_ERROR_INVALID_ARGUMENT URI but no longer.
The message types can be used when implementing a Authorizer.

@jcelliott
Copy link
Owner Author

The net/http package has server and client related code in it for example.

Good point.

I used WAMP_ERROR_INVALID_ARGUMENT URI but no longer.

If the errors should remain exported, we should probably make the names more idiomatic. Like ErrInvalidArgument, etc.

The message types can be used when implementing a Authorizer.

What if we put the message and error types in a wamp subpackage? Then you would use, e.g., wamp.ErrInvalidArgument and wamp.Invocation if you need them.

@marshauf
Copy link
Collaborator

marshauf commented Sep 3, 2015

Yes, all characters upper case is not idiomatic. Should be changed like you wrote.

The net/http package has all it's error values and status codes in the same package. I don't think the turnpike package is cluttered.

To improve and make the package more idiomatic, I suggest the use of: https://github.com/golang/lint
It brings up many suggestions.
In general we should run some tools like:
"go test -race"
It found 2 data races (on my local dev branch).

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

No branches or pull requests

2 participants