Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

x/build/internal/gophers: improve documentation, internal package design #27631

dmitshur opened this issue Sep 12, 2018 · 2 comments


None yet
3 participants
Copy link

commented Sep 12, 2018


Total mess, but a functional mess, and a starting point for the future.
— Commit 891b12dc

The gophers package is currently hard to use and hard to modify. It's not easy to read its documentation and start using it:

// (no documentation)
func GetPerson(id string) *Person

I've used and modified it multiple times, and each time, I had to read its internal code to figure out:

  • what kind of value can "id" be?
  • what is its exact format?
    • is leading '@' required for GH usernames? optional? unneeded?
  • is it case sensitive or not?
  • in what order/what type of information to add to the addPerson(...) lines?

Despite being an internal package, gophers is an important package providing value to 4 other packages, and potentially becoming used in more places. It's no longer just for computing stats, but also for tracking package owners and assigning reviews. Being internal means we can change it easily (even break the API if needed) if we come to agreement on an improved design.

Proposed Solution

I think it can be made easier to use by:

  • documenting it (so its godoc is all you need to use it, no need to read code)

    For example:

    // GetPerson looks up a person by id and returns one if found, or nil otherwise.
    // The id is case insensitive, and may be one of:
    // 	- full name ("Brad Fitzpatrick")
    // 	- GitHub username ("@bradfitz")
    // 	- Gerrit <account ID>@<instance ID> ("5065@62eb7196-b449-3ce5-99f1-c037f21e1705")
    // 	- email ("")
    func GetPerson(id string) *Person

    @bradfitz If you prefer not to be used as an example, let me know, and we can use someone else (I'm happy to volunteer) or use a generic name. But I think a well known real user makes for a better example.

Made easier to modify by:

  • making its internal addPerson logic more explicit rather than implicit

    For example, instead of what we have now:

    addPerson("Filippo Valsorda", "", "6195@62eb7196-b449-3ce5-99f1-c037f21e1705")
    addPerson("Filippo Valsorda", "")
    addPerson("Filippo Valsorda", "", "11715@62eb7196-b449-3ce5-99f1-c037f21e1705")
    addPerson("Filippo Valsorda", "", "", "@FiloSottile")
    // what kind of changes should be done to modify the end result Person struct?

    It could be something more explicit, along the lines of:

    	Name:      "Filippo Valsorda",
    	GitHub:    "FiloSottile",
    	Gerrit:    "",
    	GerritIDs: []int{6195, 11715}, // Gerrit account IDs.
    	GitEmails: []string{
    	gomote: "valsorda", // Gomote user.

    The intention is to make it easy for people to manually add and modify their entries, with predictable results, while still being able to to use code generation (ala gopherstats -mode=find-gerrit-gophers) to add missing entries.

This is just a quick draft proposal, not necessarily the final API design. If the general direction is well received but there are concerns or improvement suggestions, I'm happy to flesh it out and incorporate feedback. I wouldn't send a CL until I have a solid design.

/cc @bradfitz @andybons


This comment has been minimized.

Copy link

commented Sep 13, 2018

Given that this API is internal and you're only altering internals, I wouldn't spend a crazy amount of time worrying about implementation details. Do what you think is best for now and you can change it later on.

@dmitshur dmitshur added NeedsFix and removed NeedsDecision labels Sep 13, 2018

@dmitshur dmitshur self-assigned this Sep 13, 2018


This comment has been minimized.

Copy link

commented Sep 14, 2018

Change mentions this issue: internal/gophers: restore valid Gerrit emails

gopherbot pushed a commit to golang/build that referenced this issue Sep 14, 2018

internal/gophers: restore valid Gerrit emails
CL 132995 made a lot of changes to the gophers table based on
the output of gopherstats -mode=find-gerrit-gophers, which is
a new cmd/gopherstats mode added in that very CL.

The current implementation of Person.mergeIDs makes it so that
the first email seen is considered the Gerrit email.

Many of the additions of CL 132995 did not take that into account,
causing the Gerrit field to change for some gophers incorrectly.

Overall, that CL caused the following changes to gophers:

	Github field changes: GitHub="" -> "TomOnTime"
	Carl Henrik Lunde: GitHub="" -> "chlunde"
	Dieter Plaetinck: GitHub="" -> "Dieterbe"
	Diogo Pinela: GitHub="" -> "dpinela"
	Frank Schroeder: GitHub="" -> "magiconair"
	Gregory Man: GitHub="" -> "gregory-m"
	Jan Berktold: GitHub="" -> "JanBerktold"
	Jean de Klerk: GitHub="" -> "jadekler"
	Josselin Costanzi: GitHub="" -> "josselin-c"
	Martin Garton: GitHub="MartinGarton" -> "mjgarton"
	Matt Harden: GitHub="" -> "nerdatmath"
	Michael Darakananda: GitHub="" -> "pongad"
	Mostyn Bramley-Moore: GitHub="" -> "mostynb"
	Nicholas Maniscalco: GitHub="" -> "nicholasmaniscalco"
	Roland Illig: GitHub="" -> "rillig"
	Yasha Bubnov: GitHub="" -> "ybubnov"
	Zheng Xu: GitHub="" -> "Zheng-Xu"
	ltnwgl: GitHub="" -> "gengliangwang"
	oiooj: GitHub="" -> "oiooj"
	thoeni: GitHub="" -> "thoeni"

	Gerrit field changes:
	Andrew Bonventre: Gerrit="" -> ""
	Carl Mastrangelo: Gerrit="" -> ""
	Chris McGee: Gerrit="" -> ""
	Eric Lagergren: Gerrit="" -> ""
	Filippo Valsorda: Gerrit="" -> "6195@62eb7196-b449-3ce5-99f1-c037f21e1705"
	Guillaume J. Charmes: Gerrit="" -> ""
	Harshavardhana: Gerrit="" -> ""
	Jean de Klerk: Gerrit="" -> ""
	Joe Tsai: Gerrit="" -> ""
	Martin Möhrmann: Gerrit="" -> ""
	Matthew Dempsky: Gerrit="" -> ""
	Olivier Poitrey: Gerrit="" -> ""
	Paul Jolly: Gerrit="" -> ""
	Ralph Corderoy: Gerrit="" -> ""
	Raul Silvera: Gerrit="" -> ""
	Richard Miller: Gerrit="" -> ""
	Sebastien Binet: Gerrit="" -> ""
	Tobias Klauser: Gerrit="" -> ""
	Vitor De Mario: Gerrit="" -> ""

	Googler field changes:
	Aaron Kemp: Googler=false -> true
	Jason Hall: Googler=false -> true
	Jean de Klerk: Googler=false -> true

	(It also caused many emails to be added, but I'm not considering
	those changes since they're not relevant to golang/go#27517 and
	aren't causing harm.)

All of the Github and Googler field changes are good,
but not all of the Gerrit field changes are good.
I've manually checked them against the Gerrit server,
and classified them as follows:

	bad  Andrew Bonventre: Gerrit="" -> ""
	bad  Carl Mastrangelo: Gerrit="" -> ""
	ok   Chris McGee: Gerrit="" -> ""
	bad  Eric Lagergren: Gerrit="" -> ""
	bad  Filippo Valsorda: Gerrit="" -> "6195@62eb7196-b449-3ce5-99f1-c037f21e1705"
	bad  Guillaume J. Charmes: Gerrit="" -> ""
	bad  Harshavardhana: Gerrit="" -> ""
	ok   Jean de Klerk: Gerrit="" -> ""
	bad  Joe Tsai: Gerrit="" -> ""
	bad  Martin Möhrmann: Gerrit="" -> ""
	bad  Matthew Dempsky: Gerrit="" -> ""
	ok   Olivier Poitrey: Gerrit="" -> ""
	bad  Paul Jolly: Gerrit="" -> ""
	bad  Ralph Corderoy: Gerrit="" -> ""
	bad  Raul Silvera: Gerrit="" -> ""
	ok   Richard Miller: Gerrit="" -> ""
	bad  Sebastien Binet: Gerrit="" -> ""
	bad  Tobias Klauser: Gerrit="" -> ""
	bad  Vitor De Mario: Gerrit="" -> ""

I considered any -> changes as bad.
For the rest, it was based on which email was recognized by the
Gerrit server and had more activity overall, as well as recently.

This CL undoes all the bad Gerrit field changes, reverting them
to their original pre-CL 132995 values. It also changes the Gerrit
email for Gopherbot, and cleans up my own entry. That leaves just:

	Gerrit field changes:
	Chris McGee: Gerrit="" -> ""
	Jean de Klerk: Gerrit="" -> ""
	Olivier Poitrey: Gerrit="" -> ""
	Richard Miller: Gerrit="" -> ""

In future CLs, we'll need to be careful with the order in which
emails are added, until golang/go#27631 is resolved.

Updates golang/go#27631.
Fixes golang/go#27517.

Change-Id: I6bd289af6ea2c50c293c4576de3873658994b98a
Reviewed-by: Andrew Bonventre <>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.