define struct types instead of using anonymous structs #1900

fho · 2021-06-18T12:59:45Z

Some fields were defined inline as anonymous structs.
This made it tedious to use these structs in testcases, because
the anonymous type needed to be redefined (incl the exact same JSON
tags) whenever it was assigned.

Using anonymous struct also does not scale well when the struct changes.
For example when a field is added to an anonymous struct, every place
where the anonymous struct is assigned must also be changed.

Another advantage of using regular types is that accessors will be
generated automatically and can be used instead of having to manually
check for nil values when accessing the struct fields.

Define the types as regular structs instead.

PR Status: PR is finished but I noticed that my company hasn't signed the CA yet. As soon as it is done, I'll transition the PR to "ready for review".

The types of the fields of the EditChange struct were defined inline as anonymous structs. This made it tedious to use the EditChange struct in testcases, because the anonymous type needed to be redefined (incl the exact same JSON tags) whenever it was assigned. The TestEdit* testcases are good example for it. Using anonymous struct also does not scale well when the struct changes. For example when a field is added to an anonymous struct, every place where the anonymous struct is assigned must also be changed. Another advantage of using regular types is that accessors will be generated automatically and can be used instead of having to manually check for nil values when accessing the struct fields. Define the types as regular structs in event_types.go instead.

codecov · 2021-06-18T14:05:40Z

Codecov Report

Merging #1900 (684b1c9) into master (75644ea) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1900   +/-   ##
=======================================
  Coverage   97.65%   97.65%           
=======================================
  Files         105      105           
  Lines        6786     6786           
=======================================
  Hits         6627     6627           
  Misses         86       86           
  Partials       73       73

Impacted Files	Coverage Δ
github/activity.go	`100.00% <ø> (ø)`
github/event_types.go	`100.00% <ø> (ø)`
github/github.go	`97.01% <ø> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 75644ea...684b1c9. Read the comment docs.

gmlewis

While you are here, do you want to do the same thing for:

ProjectChange
ProjectCardChange
ProjectColumnChange
TeamChange
RepositoryVulnerabilityAlertEvent

?

github/event_types.go

Co-authored-by: Glenn Lewis <6598971+gmlewis@users.noreply.github.com>

fho · 2021-06-18T15:02:19Z

@gmlewis I also replaced the other anonymous struct types.

gmlewis

Thank you, @fho !
LGTM.

Awaiting second LGTM before merging.

gmlewis · 2021-06-18T15:32:54Z

Also, @fho - could you please remove the "Draft" setting when you are ready?

fho · 2021-06-18T15:41:16Z

@gmlewis I'll remove the draft status as soon as my employer signed the CA, probably beginning of next week. I only noticed that it was missing after I created it. :-)

gmlewis · 2021-06-18T15:44:31Z

@gmlewis I'll remove the draft status as soon as my employer signed the CA, probably beginning of next week. I only noticed that it was missing after I created it. :-)

Interesting - the googlebot marked the PR as having passed the CLA test. OK, sounds good.

fho · 2021-06-18T18:07:18Z

@gmlewis I'll remove the draft status as soon as my employer signed the CA, probably beginning of next week. I only noticed that it was missing after I created it. :-)

Interesting - the googlebot marked the PR as having passed the CLA test. OK, sounds good.

It's because I signed the CA for myself already in the past.

fho · 2021-06-29T07:20:26Z

@gmlewis my employer signed the CLA now also.
I can't see it on https://cla.developers.google.com/clas for my account but I'm in the contributors google-group of the CLA.

Update: now it shows up, yesterday was still one signature missing

gmlewis · 2021-06-29T14:35:33Z

Friendly ping, @wesleimp

sagar23sj · 2021-07-07T16:04:09Z

If we are expecting to replace all anonymous structs with defined struct types, then I guess a few are still missing in this PR.
I am listing them below :

File: github.go / ErrorResponse
File: activity.go / Feeds

@gmlewis

fho · 2021-07-08T08:15:24Z

@sagar23sj thanks, I'll update the PR and also replace those anonymous structs.

It was not my initial intent of the PR to replace all anonymous structs but it makes sense to have a consistent solution.

…ucts Fix wrong struct name in godoc comment

This fixes the following golint warning: update-urls/main.go:658:1: receiver name ep should be consistent with previous receiver name e for Endpoint

fho · 2021-07-08T08:55:41Z

done

gmlewis

Thank you, @fho !
LGTM.

Awaiting second LGTM before merging.

Parker77

LGTM.

gmlewis · 2021-07-08T17:52:42Z

@fho - would you like to resolve the merge conflicts, or would you like me to do that? I'm fine either way.

fho · 2021-07-08T18:03:21Z

@gmlewis also both fine with me, feel free to do changes :-)
Otherwise I can solve the conflict tomorrow

google-cla bot added the cla: yes Indication that the PR author has signed a Google Contributor License Agreement. label Jun 18, 2021

gmlewis requested changes Jun 18, 2021

View reviewed changes

github/event_types.go Outdated Show resolved Hide resolved

github/event_types.go Outdated Show resolved Hide resolved

Apply suggestions from code review

2256293

Co-authored-by: Glenn Lewis <6598971+gmlewis@users.noreply.github.com>

gmlewis added the Breaking API Change PR will require a bump to the major version num in next release. Look here to see the change(s). label Jun 18, 2021

fho changed the title ~~editChange event: define struct types instead of using anonymous structs~~ define struct types instead of using anonymous structs Jun 18, 2021

fho added 5 commits June 18, 2021 17:01

projectChange: define struct types instead of using anonymous structs

4f7ea12

projectCardChange: define struct types instead of using anon. structs

6d82637

projectColumnChange: define struct types instead of using anon. structs

99c9b84

teamChange: define struct types instead of using anonymous structs

f3a60ad

VulnerabilityAlert: define struct types instead of using anon. structs

82f2305

gmlewis approved these changes Jun 18, 2021

View reviewed changes

gmlewis requested a review from wesleimp June 24, 2021 00:10

fho marked this pull request as ready for review June 29, 2021 07:20

sagar23sj mentioned this pull request Jul 7, 2021

Add test cases for JSON resource marshaling #1902

Merged

fho added 4 commits July 8, 2021 10:48

feeds: define struct types instead of using anonymous structs

19e2a7b

errorResponse: define struct types instead of using anonymous structs

30283c9

fixup! teamChange: define struct types instead of using anonymous str…

56aff56

…ucts Fix wrong struct name in godoc comment

updateurls: use consistent method receiver name for Endpoint struct

684b1c9

This fixes the following golint warning: update-urls/main.go:658:1: receiver name ep should be consistent with previous receiver name e for Endpoint

gmlewis approved these changes Jul 8, 2021

View reviewed changes

Parker77 approved these changes Jul 8, 2021

View reviewed changes

sagar23sj mentioned this pull request Jul 8, 2021

Add test cases for JSON resource marshaling #1916

Closed

gmlewis added a commit that referenced this pull request Jul 8, 2021

Merge #1900

c2ddceb

gmlewis merged commit 684b1c9 into google:master Jul 8, 2021

gmlewis mentioned this pull request Jul 9, 2021

Bump version of go-github library #1957

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

define struct types instead of using anonymous structs #1900

define struct types instead of using anonymous structs #1900

fho commented Jun 18, 2021 •

edited

Loading

codecov bot commented Jun 18, 2021 •

edited

Loading

gmlewis left a comment

fho commented Jun 18, 2021

gmlewis left a comment

gmlewis commented Jun 18, 2021

fho commented Jun 18, 2021

gmlewis commented Jun 18, 2021

fho commented Jun 18, 2021 •

edited

Loading

fho commented Jun 29, 2021 •

edited

Loading

gmlewis commented Jun 29, 2021

sagar23sj commented Jul 7, 2021

fho commented Jul 8, 2021 •

edited

Loading

fho commented Jul 8, 2021

gmlewis left a comment

Parker77 left a comment

gmlewis commented Jul 8, 2021

fho commented Jul 8, 2021

define struct types instead of using anonymous structs #1900

define struct types instead of using anonymous structs #1900

Conversation

fho commented Jun 18, 2021 • edited Loading

codecov bot commented Jun 18, 2021 • edited Loading

Codecov Report

gmlewis left a comment

Choose a reason for hiding this comment

fho commented Jun 18, 2021

gmlewis left a comment

Choose a reason for hiding this comment

gmlewis commented Jun 18, 2021

fho commented Jun 18, 2021

gmlewis commented Jun 18, 2021

fho commented Jun 18, 2021 • edited Loading

fho commented Jun 29, 2021 • edited Loading

gmlewis commented Jun 29, 2021

sagar23sj commented Jul 7, 2021

fho commented Jul 8, 2021 • edited Loading

fho commented Jul 8, 2021

gmlewis left a comment

Choose a reason for hiding this comment

Parker77 left a comment

Choose a reason for hiding this comment

gmlewis commented Jul 8, 2021

fho commented Jul 8, 2021

fho commented Jun 18, 2021 •

edited

Loading

codecov bot commented Jun 18, 2021 •

edited

Loading

fho commented Jun 18, 2021 •

edited

Loading

fho commented Jun 29, 2021 •

edited

Loading

fho commented Jul 8, 2021 •

edited

Loading