Update Everything

[joepurdy]

Had some thoughts while working on https://github.com/customerio/go-customerio/pull/18 about cleaning this library up. Rather than open a major PR I figured I'd discuss first.

Small Things

LICENSE update

The LICENSE lists a copyright year of 2015, seems like a simple update to bump that to 2020

README fixes

There are lots of typos/out of date references in the README. Some choice examples:

A golang client for the Customer.io event API. Tested with Go1.10

• In reality this is being tested against 1.10, 1.11, and 1.12. Should also consider testing for current release - 3 (ie: drop 1.10 and 1.11, add 1.13 and 1.14)
• Link on "event API" is https://app.customer.io/api/docs/index.html
• That's a big 404, should be https://customer.io/docs/api/

Godoc here: godoc.org/github.com/customerio/go-customerio

• should change to link to https://pkg.go.dev/github.com/customerio/go-customerio?tab=doc
• also should just make this a GoDoc badge like the CircleCI status,

I'm not going to list all the typos, here's a lightning round of out of date links:

• http://customer.io/docs/basic-integration.html
• https://manage.customer.io/integration
• https://learn.customer.io/recipes/anonymous-invite-emails.html

Medium Things

Better test structure

Currently this is a one package show. The tests are part of package customerio. That's fine honestly, but if I had my way I'd change it to package customerio_test.

I figure the reason this isn't the case is because the tests are using private helpers from helpers.go.

This is probably not worth addressing.

Another point of contention is that the tests are tightly coupled to requiring active API credentials and they test by mutating real data. A mock server and httptest would be nice to have so that the tests weren't so tightly coupled to a real account.

Large Things

Go Modules

Not really that large, but should get the library upgraded to use Go Modules. This should be very easy actually since it's a simple package that has no third-party imports. Might as well do get it done though so that in case third-party dependencies get introduced at some point they're versioned in a sane way instead of having a vendor directory monorepo of horrors.

Refactor the customer ID encoding

PR https://github.com/customerio/go-customerio/pull/11 added a helper func called encodeID(). This smells funky to me. For starters it's pulling in Go std lib code which is weird and fragile. It also tightly couples the tests to the customerio package.

I'm sure there was a good reason for why this was necessary, but that good decision was made nearly 2 years ago so could be worthwhile to review and consider if there's a better way.

---

Happy to discuss these items and open PRs for some of them. The small stuff all seems non-controversial and worth doing no matter what. The medium and large stuff likely warrants some discussion.

[jcobhams]

@joepurdy I was about to go write a new wrapper for the API when I saw this issue. I agree it should be changed and a few improvements could be made to the package.

1. Interfaces. As is, it's a bit difficult to mock/swap the lib for something else when integrated into a project because there's no interface to implement.

2. Initializations: NewCustomerIO and NewAPIClient seem a bit strange (to me at least). Wouldn't it be cleaner/nicer to have a single New function that exposes a instance.Track.Behaviour() and instance.Api.Behaviour()

3. Rate Limiting: The API has 100rps limit but the lib does not offer any built in support.

Plus all the other things you mentioned above.

Cheers

[joepurdy]

I addressed some of the outstanding README issues in #34 and we've since added module support back in #25

I'm going to take a more active role as the maintainer of our Go library and I'll carve out better issues to track the feedback @jcobhams shared before closing this issue out. There's definitely room for improving the library quite a bit and better to do it iteratively instead of giant "rewrite the world" PRs.

[joepurdy]

@jcobhams I logged https://github.com/customerio/go-customerio/issues/36 to track work around adding an optional rate limiter. It's probably interesting to you to learn more about how the rate limits actually work (they aren't real).

I go into a bit more detail on that issue as to why we advertise a limit even though we never throttle folks. I think there are cases where it's worthwhile to guard against an out of control integration so it's not a bad idea to at least have an optional rate limiter.

Could you log an issue for your feedback on wanting an interface to implement against to give me some more context on what you're looking for here? I think I have a good idea based on other libraries I've integrated with that do provide an interface type, but would be good to hear it from you since you're more in tune with what you're looking for.