-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Support array type in go generate with whitelist #2776
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2776 +/- ##
=======================================
Coverage 98.05% 98.05%
=======================================
Files 132 132
Lines 11638 11638
=======================================
Hits 11412 11412
Misses 154 154
Partials 72 72 |
There was a problem hiding this 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?
@gmlewis |
support array type in go generate with whitelist
6fdb39d
to
ff7fd8c
Compare
There was a problem hiding this 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.
Thank you, @valbeat ! |
Fixes: #2425
This PR is similar to the following PR: