twice "." because of some people have "Google. Inc."

why not a regexp? i.e.

const suffixRe := regexp.MustCompile(`\s*(\.com|[,.]?\s*inc\.?)\s*$`)
suffixRe.ReplaceAllString(company, "")

switched to regexp on my own even though I had missed this comment !

nit: generally globals aren't prefixed with anything in golang

got rid of that cache, doesn't matter in this case given we do http requests and it hinders testability of the file

nit: unnecessary variable succ

? it's used twice (line 79 and 80)

style preference. feel free to ignore.

use pointers? companiesMap := make(map[string]*CoCounts)

@ijsnellf thanks a lot for all the educational comments, curious about this one : Why a 64 bit pointer + alloc is better than 2 64 bits ints inlined and movable ?
(I could make them 32 bits)

If you used a pointer to CoCounts as the value of the map, you wouldn't need to do the stuff with the tmp variable below. I wasn't considering memory efficiency.

but instead of the tmp I would need to test for nil and allocate a cell, I think having a small by value struct is cleaner in this case no ? (just that I wish go would do the tmp behind the scenes automagically)

since we know the number of companies, might as well pre-allocate the array
sortedCos := make([]string, 0, len(companiesMap))

done, thanks!

doesn't need to be public

changed

doesn't need to be public

same comment for many of the funcs below

it could be reused though no ? is there a big downside to being public ?

why not just strings.TrimSuffix()? The logic is exactly the same but in fewer lines. No need to reinvent.

Thank you ! I didn't know / missed that one while searching late :-)

no need to include these commented out debug statements (here and elsewhere).

done/removed

why not make this a flag (and controllable from the commandline) ?

hadn't learned about flags in go yet, done now, thanks

any reason to embed the type in the method itself?

only used in that scope ? renamed to coCounts

This is generally not golang best practice. I would consider inlining all of these log.Fatal's, especially since it is not obvious that Check() actually could result in an os.Exit call.

renamed to make it clear it's a local short cut for check & die

as elsewhere, i'd prefer:

if err := json.Unmarshal(body, &repos); err != nil {
   log.Fatalf("Unable to parse json: %v", err) 
}

It would be more idiomatic, and easier to understand.

see the other comment for a "doOrDie" script like usage (which does mean the functions aren't generally reusable, as a tradeoff)

comments should end with a '.': https://github.com/golang/go/wiki/CodeReviewComments#comment-sentences

done

should you also add a bazel BUILD file with a go_binary here? that would allow bazel run :githubContrib.

done!

nit: i'd prefer just func Company(login string, contribs int64) string

done ty

do we want tmp.contributions += c + 1 instead of the two lines above?

it's 2 different variables (contributors count vs contributions count)

use: t.FatalF() instead of these two lines.

also, go idiom is got: <value>, want: <value> (reverse of the expected -> actual ordering here).

https://github.com/golang/go/wiki/CodeReviewComments#useful-test-failures:

Note that the order here is actual != expected, and the message uses that order too. Some test frameworks encourage writing these backwards: 0 != x, "expected 0, got x", and so on. Go does not.

t.FatalF aborts the test (wouldn't collect further errors) and would have the wrong line number (line inside CheckEqual instead of the line of the caller)

will swap the expected and actual order

i don't think this matters here, but there is no reason to export CheckEqual. just have it be checkEqual.

understood. if/when I wrote more go tests I'd move it to a shared file

we've discussed this offline, and I won't block the review on this, but I'd prefer just inlining the comparsion here. It would makes things easier to read and debug too:

if got := companyFromUser(v.user, 42); got != v.expected {
  t.Fatalf("companyFromUser(%s) => %s; want %s", v.user, got, v.expected)
}

With such a construction, you would get better logs and avoid an unnecessary helper function that has complex logic around line numbers, etc.

I appreciate the input/thanks for not blocking on this

my take: it's used only twice right now in this file but it's still a lot more concise and readable to me - if I had to write dozens of tests (for different methods) I would really not want to have 3 lines and repeated constants all over my test code

please use t as the var name for *testing.T.

done

use something like v here instead, saving t for the testing.T

here and elsewhere, I would avoid exporting these methods. githubAPIURL, newGHRequest, bodyFrom, extractResult, prettyPrintJSON, company, etc.

I explained why in previous set of comments but ok, will unexport everything if that helps

i'm still not convinced this saves anything... and only serves to make the code less understandable.

if you added:

if <existing err assignment>; err != nil {
   log.Fatalf("Could not do <action>: %v", error)
}

you would have ~ the same number of characters typed ("if", "err!=nil", "log.Fatalf", vs "checkorDie"), use the same number of lines, have better error messages, and have more idiomatic go code.

1 less line (the close brace) if I keep it as it is now (with checkOrDie) but also less repeated if - also the ; seems like cheating a bit (but maybe it's very idiomatic?)
if you don't mind I'd like to keep it

using ";" is very idiomatic. this approach is not. I don't think this is a big deal for this utility, but this is something to keep in mind going forward.

this is used in a loop, correct? why not have this be done in init()?

var companyRegex *regexp.Regexp
func init() {
	companyRegex = regexp.MustCompile("[., ]+(com|inc)[ ,.]*$| |^@")
}

From the regexp docs:

MustCompile is like Compile but panics if the expression cannot be parsed. It simplifies safe initialization of global variables holding compiled regular expressions.

so I can just do it without init then ?
another example where static would be helpful to not pollute the global namespace

maybe: company := "Unknown" so later you can just have return company ?

that wouldn't work with the reassignements nor the return points