Idiomatic Go

[technosophos]

I'm frustrated with the Pubnub Go API because it does not follow the guidelines common to all Go libraries. Would it be possible to fix the library to act more like a normal Go library?

While not an exhaustive list, the following issues violate the Go coding guidelines.

Package Name:

"By convention, packages are given lower case, single-word names; there should be no need for underscores or mixedCaps." http://golang.org/doc/effective_go.html#package-names

pubnubMessaging violates this idiom. Naming the package pubnub would be great. Even putting it in pubnub/messaging with the package name messaging would be better than what is currently here.

PubNubInit()

In idomatic Go, a constructor-like function should be called New() or NewXXX(), depending on whether there is more than one constructor per package.

Calling pubnubMessaging.PubnubInit() is not idiomatic Go code. pubnub.New() is. Using Init() is actually more confusing, as it is also idiomatic to call New().Init(), but never just Init().

Variable/Constant Names

In go, public variables begin with an uppercase letter, and private variables begin with a lowercase letter. Variables should not begin with an underscore. Typically, the naming convention for constants is that they always begin with an uppercase letter.

[geremyCohen]

@technosophos Hi Matt! Thanks for your feedback on this!

Your concerns will be addressed in the next version of the client. I'm working with our lead on an estimated release date.

geremy

[technosophos]

Great. Thanks. I can contribute pull requests/patches, but I don't want to go down that path unless I know what branch to contrib on.

[geremyCohen]

@technosophos thats really cool of you, thank you. Feel free to fork, then branch on hotfix-issue2

@technosophos I had the same problem with the code as I attempted to describe in issue #1 and started making my own client https://github.com/qedus/pubnub which hopefully I can just use as a stopgap until the updated official one. One thing I really needed was to be able to replace the http.Transport Dial function with my own which the current client does not allow.

Any help with my pubnub client would be greatly appreciated, especially pernickety inputs to help create a nice idiomatic interface.

[geremyCohen]

We're going to be making some changes to the client for better naming conventions, etc. by beginning of next week, so please standby :)

@geremyCohen brilliant!

[crimsonred]

Hi @technosophos , @qedus ,

As our first step, we have made some modifications to our GO API so that it is more idiomatic with golang conventions. The code has been checked in on the branch PN-368.

https://github.com/pubnub/go/tree/PN-368.

The following changes have been made:

1. Modified the package name from "pubnubMessaging" to "messaging".
2. Exposed only the necessary functions.
3. Modified the names of the private and the public functions
4. Modified the names of global variables
5. Modified the name of the constructor from "PubNubInit" to "NewPubnub"

Please have a look and let us know your comments.

@qedus, for your comment "One thing I really needed was to be able to replace the http.Transport Dial function with my own which the current client does not allow". Can you please let me know your requirement to do this?

In the API there is a logic to reuse the transports. 2 transports are created and re-used, one for Subscribe requests (subscribe/presence calls) and the other for non-subscribe requests (here now, detailed history, time etc.). As you can see we can set the timeouts and the proxy used in the transport using the properties exposed by the API:

• messaging.SetSubscribeTimeout
• messaging.SetNonSubscribeTimeout
• messaging.SetConectTimeout
• messaging.SetProxy

If there is any other setting that you need, we can expose that setting as a property as well. As per you reply we can include the changes in the future improvements.

Thanks!

Hi @crimsonred ,

That's great news.

The reason I wanted to replace the http.Transport Dial function was so I could use pubnub with Google App Engine. In fact it turned out that I needed the ability to replace the whole http.Client. If you look at the pubnub client I made in the meantime https://github.com/qedus/pubnub you can see how I have done this.

func New(creds Credentials, client *http.Client, timeToken string) (*PubNub, error) {
        if client == nil {
                client = DefaultClient
        }

        if timeToken == "" {
                timeToken = "0"
        }

        uuid, err := genUUID()
        if err != nil {
                return nil, err
        }
        return &PubNub{credentials: creds,
                uuid:      uuid,
                client:    client,
                TimeToken: timeToken,
        }, nil
}

This allows me to create a new http.Client that uses appengine/urlfetch:

func createHTTPClient(c appengine.Context) *http.Client {
        return &http.Client{
                Transport: &urlfetch.Transport{
                        Context:  c,
                        Deadline: pubnub.SubscribeTimeout * time.Second,
                },
        }
}

Please feel free to use any of my code at https://github.com/qedus/pubnub as I found my API much easier to manage without the use of Go Lang channels.

[crimsonred]

Hi @qedus ,

Thanks for letting us know your requirement. We are planning to make changes to the API to make it compatible with GAE. We will keep you posted on this.

Thanks

[geremyCohen]

@technosophos any feedback on @crimsonred 's first rev discussed above?

[technosophos]

Wow! This is great work. The new version is much cleaner. I'll be integrating and testing this week (I hope). We're exploring moving some of our code from Java to Go, so this is great news.

Regarding @crimsonred 's suggesting, I am fine with his proposed modifications. Perhaps adding an explicit setter (SetHttpClient()), but having New() use a default http.Client object would be the best. That way, there's a sensible default and an easy override.

[crimsonred]

Hi @technosophos, thanks for your feedback and your suggestions, we will include your suggestions in the future improvements.