v2.0.0 with API changes & up-to-date data

[UmanShahzad]

Closes #13
Closes #16

[UmanShahzad]

Well, OK, some of the changes I want/need to make here really would be best done in a major version change, which under Go's versioning scheme will require a totally new import path (github.com/ipinfo/go/ipinfo/v2). Going to do that and then make that version more in line with how the other SDKs are done, and add all the currently existing types and some other missing features into it in the process.

[UmanShahzad]

The API is significantly simplified now and I've added all new data types for the core & ASN APIs. Need to do this before merge:

• Write some explicit test cases outside of the examples we have now.
• Write a CHANGELOG.md with what is different.

After this I want to do #12 and #15 as separate PRs, then this SDK is on-par with the Python one.

[przmv]

I'm against ASN ⇒ Asn renames:

Words in names that are initialisms or acronyms (e.g. "URL" or "NATO") have a consistent case. For example, "URL" should appear as "URL" or "url" (as in "urlPony", or "URLPony"), never as "Url". As an example: ServeHTTP not ServeHttp. For identifiers with multiple initialized "words", use for example "xmlHTTPRequest" or "XMLHTTPRequest".

This rule also applies to "ID" when it is short for "identifier" (which is pretty much all cases when it's not the "id" as in "ego", "superego"), so write "appID" instead of "appId".

— Go Code Review Comments, Initialisms

[UmanShahzad]

Thanks @pshevtsov for the review! I think all of the golint comments can be removed, it'd be better to just include that in CI or part of the workflow, or to just put 1 comment that contains the output of running the linter. I haven't been using it (only using vim + gopls), but I'll try to incorporate it now.

I strongly disagree with the opinion on initialisms, to be frank. "XMLHTTPRPCRequest" is definitely not as readable as "XmlHttpRpcRequest", for example, and you certainly run into these kinds of things in the wild. But I'll stick with the Golang opinion on this, although there isn't some strong requirement we do so.

[UmanShahzad]

For a single point of reference of golint results:

$ golint ./ipinfo/
ipinfo/asn.go:7:6: exported type AsnDetails should have comment or be unexported
ipinfo/asn.go:23:6: exported type AsnDetailsPrefix should have comment or be unexported
ipinfo/cache.go:9:6: exported type Cache should have comment or be unexported
ipinfo/cache.go:14:1: exported function NewCache should have comment or be unexported
ipinfo/cache.go:18:6: exported type EvaluatorFunc should have comment or be unexported
ipinfo/cache.go:20:1: exported method Cache.GetOrRequest should have comment or be unexported
ipinfo/client.go:163:1: exported function SetCache should have comment or be unexported
ipinfo/client.go:167:1: exported method Client.SetCache should have comment or be unexported
ipinfo/client.go:173:1: exported function SetToken should have comment or be unexported
ipinfo/client.go:177:1: exported method Client.SetToken should have comment or be unexported
ipinfo/core.go:8:6: exported type Core should have comment or be unexported
ipinfo/core.go:9:2: struct field Ip should be IP
ipinfo/core.go:26:6: exported type CoreAsn should have comment or be unexported
ipinfo/core.go:34:6: exported type CoreCompany should have comment or be unexported
ipinfo/core.go:40:6: exported type CoreCarrier should have comment or be unexported
ipinfo/core.go:46:6: exported type CorePrivacy should have comment or be unexported
ipinfo/core.go:53:6: exported type CoreAbuse should have comment or be unexported
ipinfo/core.go:62:6: exported type CoreDomains should have comment or be unexported
ipinfo/core.go:63:2: struct field Ip should be IP
ipinfo/core.go:69:6: func GetIpInfo should be GetIPInfo
ipinfo/core.go:74:18: method GetIpInfo should be GetIPInfo
ipinfo/ipinfo.go:5:1: comment on exported var DefaultClient should be of the form "DefaultClient ..."

[UmanShahzad]

For subdirs after clearing these:

$ golint ipinfo/...
ipinfo/cache/in_memory.go:11:6: exported type InMemory should have comment or be unexported
ipinfo/cache/in_memory.go:16:1: exported function NewInMemory should have comment or be unexported
ipinfo/cache/in_memory.go:23:1: exported method InMemory.WithExpiration should have comment or be unexported
ipinfo/cache/in_memory.go:28:1: exported method InMemory.Get should have comment or be unexported
ipinfo/cache/in_memory.go:36:1: exported method InMemory.Set should have comment or be unexported
ipinfo/cache/interface.go:6:2: exported var ErrNotFound should have comment or be unexported
ipinfo/cache/interface.go:9:6: exported type Interface should have comment or be unexported
$ golint example/...
example/cache/main.go:12:6: exported type DummyCacheEngine should have comment or be unexported
example/cache/main.go:16:1: exported function NewDummyCacheEngine should have comment or be unexported
example/cache/main.go:22:1: exported method DummyCacheEngine.Get should have comment or be unexported
example/cache/main.go:32:1: exported method DummyCacheEngine.Set should have comment or be unexported
example/cache/main.go:38:5: exported var DummyCache should have comment or be unexported

[UmanShahzad]

Some more things to do:

• Actually use the cache in the new functions.
• Add some basic new functions to help do reqs like "8.8.8.8/hostname". That should be the key into the cache for that particular request.

[przmv]

@UmanShahzad You can run golint ./... to lint CWD and subdirs.

[przmv]

@UmanShahzad Maybe let's try to use mage instead of scripts/*.sh, WDYT?

[UmanShahzad]

You can run golint ./... to lint CWD and subdirs.

Yea noticed that a bit late, hah 👍

Maybe let's try to use mage instead of scripts/*.sh, WDYT?

I don't think that's useful for us given we basically just have 1-line scripts, and that this tool is really trying to be a Makefile replacement using Golang, which is really meant for running dependency-graph oriented tasks, not a simple lint or format task.

edit: more simply, adding tools like that just increases our problems rather than actually solving anything.

[UmanShahzad]

Note: another change we're making here is requiring cache implementations themselves to be concurrency-safe, because it doesn't make sense to handle locks/etc at a higher level when the underlying impl is already doing that. The included in-memory impl is already doing this, for example.

[UmanShahzad]

@pshevtsov Can you give this a last look? I want to ship this by early next week at most.

I didn't add explicit test cases; the examples have & and are currently serving as test cases just fine for now I suppose.

[UmanShahzad]

@pshevtsov ping

[przmv]

Will review today!

[przmv]

It's neither idiomatic nor necessary to put Get into the getter's name.

— Effective Go

So, let's drop Get from the following functions' names:

$ grep -re 'func.*Get[^(]' *
ipinfo/core.go:func GetIPInfo(ip net.IP) (*Core, error) {
ipinfo/core.go:func (c *Client) GetIPInfo(ip net.IP) (*Core, error) {
ipinfo/core.go:func GetIPAddr() (string, error) {
ipinfo/core.go:func (c *Client) GetIPAddr() (string, error) {
ipinfo/core.go:func GetIPHostname(ip net.IP) (string, error) {
ipinfo/core.go:func (c *Client) GetIPHostname(ip net.IP) (string, error) {
ipinfo/core.go:func GetIPCity(ip net.IP) (string, error) {
ipinfo/core.go:func (c *Client) GetIPCity(ip net.IP) (string, error) {
ipinfo/core.go:func GetIPRegion(ip net.IP) (string, error) {
ipinfo/core.go:func (c *Client) GetIPRegion(ip net.IP) (string, error) {
ipinfo/core.go:func GetIPCountry(ip net.IP) (string, error) {
ipinfo/core.go:func (c *Client) GetIPCountry(ip net.IP) (string, error) {
ipinfo/core.go:func GetIPLocation(ip net.IP) (string, error) {
ipinfo/core.go:func (c *Client) GetIPLocation(ip net.IP) (string, error) {
ipinfo/core.go:func GetIPOrg(ip net.IP) (string, error) {
ipinfo/core.go:func (c *Client) GetIPOrg(ip net.IP) (string, error) {
ipinfo/core.go:func GetIPPostal(ip net.IP) (string, error) {
ipinfo/core.go:func (c *Client) GetIPPostal(ip net.IP) (string, error) {
ipinfo/core.go:func GetIPTimezone(ip net.IP) (string, error) {
ipinfo/core.go:func (c *Client) GetIPTimezone(ip net.IP) (string, error) {
ipinfo/asn.go:func GetASNDetails(asn string) (*ASNDetails, error) {
ipinfo/asn.go:func (c *Client) GetASNDetails(asn string) (*ASNDetails, error) {

WDYT?

[przmv]

Some of the examples fail for me:

$ go run example/get-city/main.go 
2020/12/15 17:05:09 invalid character 'D' looking for beginning of value
exit status 1

$ go run example/get-country/main.go 
2020/12/15 17:06:23 invalid character 'R' looking for beginning of value
exit status 1

$ go run example/get-location/main.go 
2020/12/15 17:07:45 json: cannot unmarshal number into Go value of type string
exit status 1

$ go run example/get-org/main.go 
2020/12/15 17:08:22 invalid character 'A' looking for beginning of value
exit status 1

$ go run example/get-postal/main.go 
2020/12/15 17:08:50 json: cannot unmarshal number into Go value of type string
exit status 1

$ go run example/get-region/main.go 
2020/12/15 17:09:16 invalid character 'R' looking for beginning of value
exit status 1

$ go run example/get-timezone/main.go 
2020/12/15 17:09:52 invalid character 'E' looking for beginning of value
exit status 1

[UmanShahzad]

It's neither idiomatic nor necessary to put Get into the getter's name.

This is talking about getters & setters for fields on a struct, like how it's often done in Java. The "GetX" functions here are not those kinds of functions.

Let's apply Keep a Changelog format. WDYT?

I can see the benefit of a standardized form for a big, changing project, but for this very small-scoped project which will hardly change much after this PR and the existing issues, it doesn't seem necessary, and just gets in the way of being able to explain what we want without having to bin-pack our explanations into these categories.

Should be MCC and MNC (all upper case), or even better MobileCountryCode and MobileNetworkCode

We can upper-case, but let's keep the abbreviations; that's what the other SDKs do, and is what the API does.

Should be VPN

👍

Some of the examples fail for me:

Thanks for catching that; it seems that since IPinfo returns text rather than a valid JSON string (which would have the text wrapped in "), the JSON parser doesn't parse it properly.

I checked to see if we have something in the backend currently to give back actual JSON on e.g. curl ipinfo.io/city -H "Accept: application/json", but we don't, and changing that will likely be backwards incompatible at this point for many people who may automatically be attaching that Accept header but parsing the text raw. We're gonna have to parse the text raw as well; no big deal. (see next comment).

I don't like using cache and token arguments here. It leads to ugly code like the following to initialize API client with the default HTTP client, no cache, and no token:

Sure but what is the alternative? Golang doesn't support method overload or default argument values. We would have to complicate the API significantly for this. Or the user would have to add an extra line to their code to call set methods for each of the missing fields.

The function in question is also likely to be used in only 1 place throughout a user's codebase, or at least highly infrequently. Being explicit about what you want upfront by stating all the arguments seems perfectly reasonable to me for such a function. Golang by its design doesn't seem to have chosen to make this use case pretty in any case.

[UmanShahzad]

To update my own point about "parsing text raw": nevermind that, we'll just make a normal request and then pick out the individual field. This will have the benefit of caching the entire IP data, so if another function is used for other data on that IP, it's cached.

[UmanShahzad]

Thanks again for your review, @pshevtsov . I've fixed some of the naming issues and all the examples work now.

I'm going to merge this now and consider v2.0.0 ready.