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

Support array type in go generate with whitelist #2776

Merged
merged 2 commits into from May 14, 2023

Conversation

olibaa
Copy link
Contributor

@olibaa olibaa commented Apr 30, 2023

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
@codecov
Copy link

codecov bot commented Apr 30, 2023

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           

Copy link
Collaborator

@gmlewis gmlewis left a comment

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?

github/gen-accessors.go Outdated Show resolved Hide resolved
@olibaa
Copy link
Contributor Author

olibaa commented May 1, 2023

@gmlewis
Sounds good!
I will fix the code.

@olibaa olibaa changed the title Support array type in go generate Support array type in go generate with whitelist May 1, 2023
support array type in go generate with whitelist
@olibaa olibaa force-pushed the support-array-type-go-generate branch from 6fdb39d to ff7fd8c Compare May 1, 2023 02:39
@olibaa olibaa requested a review from gmlewis May 1, 2023 02:49
@gmlewis gmlewis added the NeedsReview PR is awaiting a review before merging. label May 1, 2023
Copy link
Collaborator

@gmlewis gmlewis left a comment

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.

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

gmlewis commented May 14, 2023

Thank you, @valbeat !
Merging.

@gmlewis gmlewis merged commit 60429b4 into google:master May 14, 2023
9 checks passed
Sign up for free to 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