Bugfix: REST publish encoding was "utf8" should be "utf-8"

[CraigChilds94]

There was an error which prevented me from running tests and vetting the application. The package name "internal" is not valid and Go v1.8 complains. I've moved the package up 1 directory so instead of /ably/internal/ablyutil it's /ably/ablyutil.

I've also fixed the encoding which is being set when publishing a REST message. The clients and everywhere else in the library use "utf-8" as the encoding, but this was using "utf8" which was causing an encoding error on the client when trying to decode the message.

[paddybyers]

Hi,

Thanks for this contribution! This library hasn't had much attention recently but we've got someone starting work on it from next week so hopefully you should see some progress. In the meantime we'll have a look at this PR.

Thanks

[ORBAT]

Hi @CraigChilds94! What was the exact problem you had related to the /ably/internal/ablyutil package? Can you provide a log or an example?

Internal packages have been around since Go 1.5, so it should definitely work with 1.8.

[mattheworiordan]

@ORBAT I assume we should merge in the utf-8 change regardless?

[ORBAT]

@mattheworiordan: the utf-8 change should be fine as it seems to be in line with the other client libraries. It would be a good idea to add base64, json and utf-8 as constants in ably/proto instead of using string literals, but that's not absolutely necessary for this PR

[mattheworiordan]

Ok, well perhaps raise an issue for ably/proto and cherry-pick the utf-8 commit and merge?

[mattheworiordan]

Ok, well perhaps raise an issue for ably/proto and cherry-pick the utf-8 commit and merge?

[CraigChilds94]

@ORBAT I can't remember the exact error I had now with the internal package, but I had a quick look online to see if it was just me and it looks like it happens on other packages too, other's that use internal in their name. I don't see why it'd need to be internal anyway it works fine as is :)

Thanks for taking a look though, the utf-8 thing was bugging me, as my JS client kept logging errors every time I received messages.

And using constants sounds good to me also! :)

[ORBAT]

@CraigChilds94, the rationale behind internal packages like github.com/ably/ably-go/ably/internal/ablyutil is to allow having internal utility etc. packages (or APIs, in a sense) that can be used by all packages under the github.com/ably/ably-go/ably/ prefix, but are not intended for use via the public API. This way package maintainers like us can change things behind the scenes without possibly breaking an API that someone was using and relying on.

[CraigChilds94]

@ORBAT Ah cool, I guess it comes down to a package scope thing. If it's just a case of tracking changes, what about putting it in a different repo?

The error I get when trying to build the package with that directory in places is use of internal package not allowed, have you encountered this before?

[ORBAT]

@CraigChilds94, code under internal is not intended for use outside of the package it resides in; moving it to a package of its own would still expose that API to the public. Check out the design doc I linked to here, it does a good job at explaining the idea behind internal packages.

Ultimately though, I realized that the problem is that we don't have up to date contribution instructions / how to work with forks of the repo. Since we use internal packages, when you fork the package, you need to add your fork as a remote in $GOPATH/src/github.com/ably/ably-go. If you clone/go get it to its own directory, you'll get exactly the error you described.

In your case the procedure would be:

cd $GOPATH/src/github.com/ably/ably-go
git remote add fork git@github.com:CraigChilds94/ably-go
git fetch fork
git checkout --track fork/bug/fix-rest-publish-encoding
# hack hack hack [...]

Sooo, long story short, I'd suggest doing ^ that, and then reverting the changes related to internal packages. The other changes are good to go.

[CraigChilds94]

@ORBAT Awesome. Thanks for that, I'll try and get to that this week.

[ORBAT]

@CraigChilds94: we're about to roll out a release with the UTF-8 fix included, so I'm closing this PR. Thank you for taking the time to do one in any case