Generate accessors for structs with pointer fields by gmlewis · Pull Request #543 · google/go-github

gmlewis · 2017-02-03T09:02:24Z

Fixes #45.

Change-Id: Ib2b6cc5d713e2eb833ee3c7fcfbd804bfe8fa313

dmitshur · 2017-02-03T16:08:41Z

Thanks for working on this @gmlewis. Before I look at the details of the implementation, I'd like to discuss this change at a higher level. I have some concerns about the general direction and I'd like to see where others stand. Let's maybe do that in #45 itself, and leave this PR to discuss implementation details. I'll ping you there.

dmitshur · 2017-02-14T00:15:47Z

As I posted in #45 (comment), my potential concerns are resolved and I have no objections to doing this. I'll leave some review comments on the implementation now.

dmitshur · 2017-02-14T00:28:51Z

First, some high level questions/thoughts for @gmlewis, looking only at the final API after this PR.

The "Get" prefix is slightly unfortunate, given Go style suggests to avoid "Get" prefix from getters (source: https://golang.org/doc/effective_go.html#Getters).

However, I understand this is unavoidable because otherwise field name and method name would have same name, right? Also, this is the same solution that protobuf uses and is therefore consistent, right?

Just mentioning this, I don't think we need to change it. Also, one way to justify the "Get" prefix is that it's not a simple getter, it has logic for returning zero value if the pointer is nil.
The documentation text for methods seems to prioritize mentioning the nil case:
```
// GetMessage returns the zero value if nil, otherwise the actual value.
```
Is that the way to go? I'd expect the nil case to be the less common path, and the actual value being the primary path. So would it better to reflect that in the docs by swapping the order? Something that mentions the actual value first, nil case second, like:
```
// GetMessage returns the Message field if it's non-nil, zero value otherwise.
```
(That phrasing doesn't sound better, but perhaps it can be tweaked. Just want to get your thoughts.)
This is actually somewhat implementation-related. Currently, this PR generates 56 "-accessors.go" files. Given they're all generated and most users/developers won't need to look at/modify the detailed contents of those files, maybe it'd be better to consolidate them all in a single generated .go file?

That way, when working on this package, there are less files get in your way that you don't want to modify. Also smaller risk of accidentally modifying a generated file, because there are fewer of them.

Compare the file list before and after this PR.

Please don't change this yet, I just want to get your thoughts on this first. We should consider the trade-offs of both approaches.

gmlewis · 2017-02-14T15:55:18Z

@shurcooL - addressing your questions/thoughts:

Yes, "Get" is consistent with the proto2 implementation.
I went through a few iterations on the comment, and am fine with changing it. Both versions seem clear to me, but am happy to emphasize as suggested.
Sure, I'm happy to consolidate to a single file... I was originally thinking it would be nice to easily jump to just the accessors for a particular file, but with all the editor tooling we have these days, maybe a single file is better after all.

willnorris · 2017-02-14T18:31:25Z

+1 to single file

…

On Tue, Feb 14, 2017 at 7:55 AM, Glenn Lewis ***@***.***> wrote: @shurcooL <https://github.com/shurcooL> - addressing your questions/thoughts: 1. Yes, "Get" is consistent with the proto2 implementation. 2. I went through a few iterations on the comment, and am fine with changing it. Both versions seem clear to me, but am happy to emphasize as suggested. 3. Sure, I'm happy to consolidate to a single file... I was originally thinking it would be nice to easily jump to just the accessors for a particular file, but with all the editor tooling we have these days, maybe a single file is better after all. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#543 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAEWG5WFHKq6kjaw6N5AdsyJ-ogUXP3ks5rcc5mgaJpZM4L2GC9> .

dmitshur · 2017-02-15T21:16:46Z

+// go-github authors should run the following commands before
+// sending PRs to GitHub:
+//
+//     cd github.com/go-github


cd github.com/go-github? That doesn't sound right.

dmitshur · 2017-02-15T21:17:58Z

+//     cd github.com/go-github
+//     go generate ./...
+//     go test ./...
+//     go vet ./...


How about deleting the cd step and making these commands use absolute import path patterns rather than relative? E.g.:

go generate github.com/google/go-github/...

Then the same command can be run from any directory, and it'll do the right thing.

dmitshur · 2017-02-15T21:23:43Z

+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+// github-accessors.go generated by gen-accessors; DO NOT EDIT


The standardized generated file disclaimer header format not official yet, but it seems to be on its way to become so. Let's start following that standard.

// Code generated by gen-accessors; DO NOT EDIT.

dmitshur · 2017-02-15T21:30:12Z

+//
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+


I think you should consider adding:

// +build ignore

It won't affect go generate functionality since the .go file is being go run directly.

But it'll prevent a poorly named and unneccessary tools binary from being placed in users $GOPATH/bin directory if they install all packages in go-github repository. Thoughts?

In fact, is there an advantage in placing this file in a separate directory, instead of just inside github folder, the same folder as the github package where it's being used?

We don't want gen-accessors.go in the same github directory because it is not in the github package and may cause confusion. It seems to be common practice to move command-line tools to a subdirectory called tools or cmd or tools/cmd or cmd/tools... I'm flexible with the directory name, but I think we want to keep it in a separate directory.

dmitshur · 2017-02-15T21:42:42Z

+// {{.Filename}} generated by gen-accessors; DO NOT EDIT
+
+package {{.Package}}
+{{if .Imports}}


It's possible to simplify the template in this instance by using {{with .Imports}} instead of {{if .Imports}}.

dmitshur · 2017-02-15T21:43:46Z

+	if err := sourceTmpl.Execute(&buf, t); err != nil {
+		return err
+	}
+	clean, err := format.Source(buf.Bytes())


Does gofmting the output from template have an effect, i.e., does it change something, or is it being done "just in case"?

If it changes something, what is it? Asking because I'm curious.

Yes, it correctly converts spaces to tabs. I worked on this project:
https://godoc.org/google.golang.org/api
and calling gofmt is standard operating procedure when generating Go code.

gmlewis · 2017-02-19T16:39:22Z

I believe I've addressed the review comments from @shurcooL.
PTAL.

willnorris · 2017-02-21T04:43:52Z

 // Use of this source code is governed by a BSD-style
 // license that can be found in the LICENSE file.

+// go-github authors should run the following commands before


these instructions should be added to CONTRIBUTING.md. Either in addition to, or probably instead of, here.

dmitshur

@gmlewis, let me reply to one of your inline comments here, because it's really hard to track conversations in inline comments in large PRs. You said:

We don't want gen-accessors.go in the same github directory because it is not in the github package and may cause confusion. It seems to be common practice to move command-line tools to a subdirectory called tools or cmd or tools/cmd or cmd/tools... I'm flexible with the directory name, but I think we want to keep it in a separate directory.

Given your latest changes (to add // +build ignore and rename directory to gen-accessors, although the directory name doesn't matter as long as there's a // +build ignore), I'm okay with the current version an don't have objections to it being merged as is.

However, I don't fully agree with what you said, so I wanted to share my perspective for your consideration. You should consider my counter-points and then proceed with what you think this the best approach.

In my experience and from what I've seen done more commonly in the Go project, its subrepos, and 3rd party projects, the general pattern that's used is roughly as follows.

When a generator tool is absolutely general and not tied to any specific package, it's provided as a standalone command, and then invoked via //go:generate cmdname.

For example, stringer, bundle, etc.
On the other extreme, when a generator tool is very specific, tied to a given package, and executed via //go:generate go run gen-something.go (i.e., go run on .go files instead of invoking a binary), placing such a file with // +build ignore and package main inside the same directory is most common, and IMO, the least confusing.

The fact that it's a package main, instead of package libraryname, and the // +build ignore comment mean that it won't be a part of the library itself. Because it's so closely tied to the package, having it in the same directory is fitting. Placing it in another directory makes it harder to know what packages actually use that file.

Some examples of this being done are:
- In golang.org/x/tools/imports package, there are .go generator files specific to that very package. See https://github.com/golang/tools/tree/8e779ee0a4509783f6940db32d1a4e8df910cc8b/imports, which contains mkindex.go and mkstdlib.go. The latter is used when generating that package at https://github.com/golang/tools/blob/8e779ee0a4509783f6940db32d1a4e8df910cc8b/imports/imports.go#L5.
- See another example in gxui project at https://github.com/google/gxui/blob/f85e0a97b3a4aafc65be1b86d962a8734cd27bf9/gxfont/gxfont.go#L14.
- Another example in golang.org/x/mobile/gl package, https://github.com/golang/mobile/blob/1089cdd3e20effaa211bc88d452a371b29072f30/gl/doc.go#L66.
- In golang.org/x/text/width package, https://github.com/golang/text/blob/767daa157b4d460bba02409ff856ddeeb8e76ab7/width/width.go#L6.
- In sort package in GOROOT, https://github.com/golang/go/blob/35ffca31b180e6f9da6035326132f048980dc58c/src/sort/sort.go#L5.
There may be some middle ground, where the generator code is project-specific (rather than completely general), but used by more than one package. Or, if the generator code is extremely complicated and makes up an entire project of its own, with inner packages. Then I think having it in a separate directory is more fitting.

However, gen-accessors.go is a small, single-file generator. It generates code very specific to the github package, and I think it therefore belongs in the github package (directory). I think having it here would not be more confusing. That way, the github package is more self-contained: every single file needed to build the package, and every file needed to generate a part of the package, are contained in the package's directory.

dmitshur · 2017-02-21T17:19:58Z

+		"ErrorResponse.GetResponse":       true,
+		"RateLimitError.GetResponse":      true,
+		"AbuseRateLimitError.GetResponse": true,
+	}


This is a question. What's the pattern of what's included in the blacklist?

Are all entries here simply methods that already exist, that happen to begin with "Get"? And the blacklist is needed to prevent the generated code from creating a duplicate method name, is that so?

The pattern is <receiver type> + "." + <method name>.

Yes, the blacklist prevents these methods from being created... either because they already exist or because they would otherwise create problems if they were to be created.

dmitshur · 2017-02-21T17:22:08Z

+	}
+
+	// Sort getters by ReceiverType.FieldName
+	sort.Sort(byName(t.Getters))


FWIW, you could use sort.Slice now to make this a little shorter (i.e., byName type and its 3 methods can go away), if you wanted to, since #554 is merged.

(Completely optional, just mentioning it.)

I just tried this with Go 1.7 and it failed.
It looks like sort.Slice is Go 1.8 only:
https://beta.golang.org/doc/go1.8#sort_slice
and since we still support Go 1.7, I will not yet switch this to sort.Slice.

Ah, yes, my mistake. I was off by one. :)

Although, this is code that runs during go generate, only by developers of this package, not users. So in theory we could require/expect developers to be running 1.8. But, this is absolutely not worth it to save a few lines. Just a fun observation.

dmitshur

LGTM.

gmlewis · 2017-02-21T18:36:22Z

Thank you for the detailed discussion regarding generators, @shurcooL!
I was not aware of this common usage, and it makes sense, so I've moved gen-accessors.go into the github directory.

gmlewis · 2017-02-24T19:50:52Z

Merging.

gmlewis · 2017-02-24T19:58:53Z

Integrated in cd756c0

dmitshur mentioned this pull request Feb 3, 2017

add Getter methods for data structures #45

Closed

gmlewis force-pushed the gen-accessors branch from e1aa8de to 50ff713 Compare February 14, 2017 23:57

dmitshur reviewed Feb 15, 2017

View reviewed changes

ojongerius mentioned this pull request Feb 16, 2017

WIP GH-56 Refactor to use pointers for all fields. zorkian/go-datadog-api#83

Merged

gmlewis force-pushed the gen-accessors branch from 50ff713 to 6c6979c Compare February 19, 2017 15:44

willnorris reviewed Feb 21, 2017

View reviewed changes

dmitshur reviewed Feb 21, 2017

View reviewed changes

dmitshur approved these changes Feb 21, 2017

View reviewed changes

gmlewis closed this Feb 24, 2017

gmlewis force-pushed the gen-accessors branch from 388cd37 to cd756c0 Compare February 24, 2017 19:56

gmlewis deleted the gen-accessors branch February 24, 2017 19:57

Conversation

gmlewis commented Feb 3, 2017

Uh oh!

dmitshur commented Feb 3, 2017

Uh oh!

dmitshur commented Feb 14, 2017

Uh oh!

dmitshur commented Feb 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gmlewis commented Feb 14, 2017

Uh oh!

willnorris commented Feb 14, 2017 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmitshur Feb 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmitshur Feb 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmlewis commented Feb 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmitshur left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmitshur left a comment

Choose a reason for hiding this comment

Uh oh!

gmlewis commented Feb 21, 2017

Uh oh!

gmlewis commented Feb 24, 2017

Uh oh!

gmlewis commented Feb 24, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dmitshur commented Feb 14, 2017 •

edited

Loading

dmitshur Feb 15, 2017 •

edited

Loading

dmitshur Feb 15, 2017 •

edited

Loading

dmitshur left a comment •

edited

Loading