Consistent Go naming for JSON fields

[gmlewis]

As discussed in #520 (comment) and #602 (comment) the naming of fields such as Total (as opposed to TotalCount) where the JSON name is total_count seems inconsistent and it would be nice to consolidate.

However, this is a breaking API change.

This issue has been opened to discuss the situation and see if it is worth breaking the API to have more consistency.

[dmitshur]

We can also consider adding a new field TotalCount that also gets decoded from `json:"total_count"`, and mark the old Total as deprecated. That way it's not a breaking API change, but it introduces a deprecated field.

[gmlewis]

Good point. I like it. Maybe a general sweep might be in order.
It would be a good project for a new contributor too.

[aleksclark]

@gmlewis I'm down for doing it either way, but #588 is already unavoidably (afaics, unless we want to return interface{} which I would personally frown on) a breaking change for an actual bug. Is there a deprecation/semver policy somewhere I've missed that can be a guide for making these sorts of calls? Perhaps the deprecate-and-replace method should be used for #588 as well?

[gmlewis]

The semver discussion is going on in #376.
Otherwise, we bump the libraryVersion number in github.go on breaking API changes.
Part of the resolution of #376 will be to document the policy.

[aleksclark]

ok, I'll proceed as described with the deprecated fields and increment the version number.

[aleksclark]

https://github.com/google/go-github/blob/master/github/activity_events.go#L19

RawPayload is used here despite the fact that the field is called payload. The struct then implements a Payload() method which is already deprecated, as well as a ParsePayload() method which is the new hotness. Seems reasonable to me to rename RawPayload to Payload and remove the deprecated method. On the other hand, this deprecation is only 23 days old. Thoughts?

[dmitshur]

IMO, it's probably for the best to leave RawPayload alone for now. It has a lot of machinery around it to parse it and convert it to something higher level, such that calling the field "raw" should not be confusing.

The only field I think is worth dealing with for now is TotalCount. @gmlewis, are there others you wanted to tackle?

[aleksclark]

Hmm, ok. I interpreted 'full sweep' to mean all json fields should be inspected for naming consistency. For instance, there is a lot of swapping of 'Repo' and 'repository', and at least one place where the field is called Repository but the json field is 'repo'.

…

On Fri, Mar 31, 2017 at 4:43 PM, Dmitri Shuralyov ***@***.***> wrote: IMO, it's probably for the best to leave RawPayload alone for now. It has a lot of machinery around it to parse it and convert it to something higher level, that calling that field "raw" should not be confusing. The only field I think is worth dealing with for now is TotalCount. @gmlewis <https://github.com/gmlewis>, are there others you wanted to tackle? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#605 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAlDnB4lqlyU_zQ389zxRW5mp5RTuuhAks5rrXN_gaJpZM4Mtrr-> .

-- Aleks Clark cell: 775-391-6114

[dmitshur]

Maybe a general sweep might be in order.

Ah, @gmlewis said that in a comment, not the original issue. I missed that.

Sure. A general sweep is okay. I think RawPayload field is not a good candidate (for reasons I stated above), but perhaps there are others that are. Let us know if you find something else.

[dmitshur]

One of my worries about a full sweep is that it might be hard to justify renaming fields that are well named but happen not to match exactly. It introduces a new deprecated field, and I'm not entirely sure it's always an improvement. But let's consider it on a case-by-case basis.

[gmlewis]

The only field I think is worth dealing with for now is TotalCount. @gmlewis, are there others you wanted to tackle?

I think TotalCount was the most obvious. Maybe whoever tackles this one could come up with a list of inconsistencies for review?

[aleksclark]

@gmlewis sounds good to me. Can you assign to me please?

…

On Fri, Mar 31, 2017 at 5:01 PM, Glenn Lewis ***@***.***> wrote: I think TotalCount was the most obvious. Maybe whoever tackles this one could come up with a list of inconsistencies for review? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#605 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAlDnPWFyebSVqgWU3O7qYtoER7PRbKDks5rrXfUgaJpZM4Mtrr-> .

-- Aleks Clark cell: 775-391-6114

[elliott-beach]

Hi, I just started working on a spreadsheet of discrepancies. Any issue with assigning to me?

[aleksclark]

Ok with me

…

On Aug 20, 2017 10:19 PM, "Elliott Beach" ***@***.***> wrote: Hi, I just started working on a spreadsheet of discrepancies. Any issue with assigning to me? — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#605 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAlDnMn0KoD9FLyRGtjUaU-UNOzjwNU5ks5saPdUgaJpZM4Mtrr-> .

[elliott-beach]

Here's a list of all inconsistencies:

filename: go-field api-field

activity_events.go: RawPayload payload
activity_star.go: Repository repo
authorizations.go: UpdateAt updated_at
event_types.go: Org organization
event_types.go: Repo repository
git_commits.go: Login username
git_trees.go: Entries tree
issues.go: PullRequestLinks pull_request
repos_deployments.go: UpdatedAt pushed_at
repos.go: DismissalRestrictionsRequest dismissal_restrictions
repos_hooks.go: Repo repository
repos_invitations.go: Repo repository
search.go: Total total_count

It seems that the use of "Repo" or "Repository" for field names is inconsistent.

The full list was generated using a script I cooked up at https://gist.github.com/e-beach/120a6f26c72c8d08f2895b933d1af8ab, I discarded these differences that seem highly intentional:

filename: go-field api-field

reactions.go: MinusOne -1
reactions.go: PlusOne +1
repos_stats.go: Additions a
repos_stats.go: Commits c
repos_stats.go: Deletions d
repos_stats.go: Week w
search.go: CodeResults items
search.go: Commits items
search.go: Issues items
search.go: Repositories items
search.go: Users items

I don't think I can take further action on this myself, but I believe the script caught all the differences by detecting API fields that not equivalent to the json field converted to camelCase.

[gmlewis]

Thanks, @e-beach! Invite sent.

[elliott-beach]

Thanks, accepted! To be clear, I don't think I can make a PR based on this as I'm not currently in a position to judge what changes should be made.

[dmitshur]

Thank you very much for contributing those investigation results in #605 (comment), @e-beach! That is extremely helpful (and often undervalued), because it will make it much easier for anyone to contribute a code fix and for us to review it. Your contribution gets a 🌟 from me! :)

We should discuss here first what changes we want to make, before working on a PR, since it'll be easier and more efficient this way.

[dmitshur]

Let's start with the easier ones. I think it's for sure that we want to fix these inconsistencies, because they're a typo:

authorizations.go: UpdateAt updated_at
repos_deployments.go: UpdatedAt pushed_at

I think a breaking API change for both is the most effective way to move forward. We should definitely do it for repos_deployments.go because it's a very misleading field name.

The one in authorizations.go is a harmless typo, but I suspect it's easier to just break the API, have people update the code by typing 1 character and compile it successfully, than to go through introducing a deprecated field, etc.

That said, perhaps introducing a second field and deprecating UpdateAt would be fine too:

UpdatedAt *Timestamp   `json:"updated_at,omitempty"`
UpdateAt  *Timestamp   `json:"updated_at,omitempty"` // Deprecated: Use UpdatedAt instead.

Edit: Not fine, see following comment.

Anyone who wants can send a PR to resolve these two. This comment is made possible thanks to @e-beach's investigation, so thanks again for that!

[dmitshur]

Actually, as I suspected, it won't be possible to use a deprecated field like that. Due to the rules for overlapping JSON fields in https://godoc.org/encoding/json:

The Go visibility rules for struct fields are amended for JSON when deciding which field to marshal or unmarshal. If there are multiple fields at the same level, and that level is the least nested (and would therefore be the nesting level selected by the usual Go rules), the following extra rules apply:

1. Of those fields, if any are JSON-tagged, only tagged fields are considered, even if there are multiple untagged fields that would otherwise conflict.

2. If there is exactly one field (tagged or not according to the first rule), that is selected.

3. Otherwise there are multiple fields, and all are ignored; no error occurs.

There will be multiple fields and they will be ignored.

See https://play.golang.org/p/MWsbl69mT8.

So it has to be a simple breaking API change.

[dmitshur]

I'll copy my #709 (comment) from the PR so we can discuss it here:

Now that I've looked, are we sure pushed_at is the right JSON field?

It doesn't look to be. I think it might be updated_at instead.

Take a look at https://developer.github.com/v3/repos/deployments/. There are 7 hits for "updated_at", 0 for "pushed_at".

Also, I just noticed there are two UpdatedAt fields with pushed_at json tag in repos_deployments.go, not one:

go-github/github/repos_deployments.go

Line 26 in 5f3187e

UpdatedAt *Timestamp `json:"pushed_at,omitempty"`

go-github/github/repos_deployments.go

Line 140 in 5f3187e

UpdatedAt *Timestamp `json:"pushed_at,omitempty"`

Both were added in c491754 by @dlapiduz 3 years ago. Any chance you remember if there was a reason for the discrepancy, or was it an oversight? I think there'd be a comment if it were on purpose, so I suspect it was unintended.

[dmitshur]

It was done in PR #139. There's no discussion about the discrepancy, so that pretty much confirms it was unintentional.

[elliott-beach]

The pushed_at field definitely does not exist.

In addition to the documentation, sending requests to https://api.github.com/repos/github/hub/deployments/16449242 (a deployment)
and
https://api.github.com/repos/github/hub/deployments/16449242/statuses (a deployment status)

shows that both instances should use updated_at in the json tag.

[dlapiduz]

@shurcooL @e-beach I don't recall exactly what happened but I think that the deployments API was alpha/beta at that time and it was changing. That might be the source of the discrepancy.

[dmitshur]

I see, that would make sense. Thanks for looking into it and providing confirmation @e-beach and @dlapiduz!

[elliott-beach]

Now that we've renamed a few fields, a decision should also be made on what, if any, other fields ought to be changed. The comments above mentioned TotalCount, My thinking is that trivial renamings that cause breaking changes for users are not worth the enhanced consistency.

[dmitshur]

Yeah. I'm going to think about it some more, but I think a likely good next step is to stop here and close this issue.

[RajatVaryani]

@dmitshur Please close the issue. It lead me to read the entire thread and in the end, I realized there's no work to be done. Thanks

[gmlewis]

Sorry for the inconvenience, @RajatVaryani.
Closing issue.