Skip to content

Support array type in go generate with whitelist - #2776

Merged
gmlewis merged 2 commits into
google:masterfrom
olibaa:support-array-type-go-generate
May 14, 2023
Merged

Support array type in go generate with whitelist#2776
gmlewis merged 2 commits into
google:masterfrom
olibaa:support-array-type-go-generate

Conversation

@olibaa

@olibaaolibaa commented Apr 30, 2023

Copy link
Copy Markdown
Contributor

Fixes: #2425

This PR is similar to the following PR:

@olibaa
olibaa marked this pull request as ready for review April 30, 2023 12:42
@olibaaolibaa mentioned this pull request Apr 30, 2023
@codecov

codecovBot commented Apr 30, 2023

Copy link
Copy Markdown

Codecov Report

Merging #2776 (2a957fc) into master (a938017) will not change coverage.
The diff coverage is n/a.

@@ Coverage Diff @@## master #2776 +/- ##
=======================================
Coverage 98.05% 98.05% =======================================
Files 132 132 Lines 11638 11638 =======================================
Hits 11412 11412 Misses 154 154 Partials 72 72 

@gmlewisgmlewis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I appreciate what you are trying to do, @olibaa, this is not how I think it should be done.

Please read very carefully what I wrote here.
I do not want to add getters for every possible slice.
They are simply unnecessary and add a huge amount of bloat for zero benefit.

What I recommend here is that you add a whitelist of fields that your code should process.
So, for example, we have only a single field currently that we want to add to the whitelist: PushEvent.Commits.

Then, on lines 140-142 of github/gen-accessors.go, you would check this whitelist and only call t.addArrayType if the current field matches an item in the whitelist (which itself could simply be a var whitelistSliceGetters = map[string]bool{ ...: true} to make lookup super-easy).

That way, when we look at the diffs of github/github-accessors.go and github/github-accessors_test.go, we should only see one new field in this PR... the one for PushEvent.Commits.

How does that sound to you?

Comment threadgithub/gen-accessors.go Outdated
@olibaa

Copy link
Copy Markdown
ContributorAuthor

@gmlewis
Sounds good!
I will fix the code.

Comment threadgithub/gen-accessors.go
@olibaaolibaa changed the title Support array type in go generateSupport array type in go generate with whitelistMay 1, 2023
support array type in go generate with whitelist
@olibaa
olibaaforce-pushed the support-array-type-go-generate branch from 6fdb39d to ff7fd8cCompareMay 1, 2023 02:39
@olibaa
olibaa requested a review from gmlewisMay 1, 2023 02:49
@gmlewisgmlewis added the NeedsReview PR is awaiting a review before merging. label May 1, 2023

@gmlewisgmlewis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beautiful, @olibaa! Well done!
LGTM.

Awaiting second LGTM+Approval from any other contributor to this repo before merging.

@gmlewisgmlewis removed the NeedsReview PR is awaiting a review before merging. label May 14, 2023
@gmlewis

Copy link
Copy Markdown
Collaborator

Thank you, @valbeat !
Merging.

@gmlewis
gmlewis merged commit 60429b4 into google:masterMay 14, 2023
jlaportebot added a commit to jlaportebot/go-github that referenced this pull request Jun 28, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

PushEvent GetCommits() Method

3 participants

@olibaa@gmlewis@valbeat