Skip to content

Add pointers to structs in slices #1443

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

Merged
merged 1 commit into from
Feb 28, 2020
Merged

Add pointers to structs in slices #1443

merged 1 commit into from
Feb 28, 2020

Conversation

gmlewis
Copy link
Collaborator

@gmlewis gmlewis commented Feb 28, 2020

Since we decided in #180 to standardize using pointers to structs when they appear in slices, and some slices have slipped through code reviews without having pointers, this PR cleans these up.

Fixes: #1100.

This is a breaking API change.

@googlebot googlebot added the cla: yes Indication that the PR author has signed a Google Contributor License Agreement. label Feb 28, 2020
@gmlewis gmlewis requested a review from gauntface February 28, 2020 00:40
Copy link
Collaborator

@wesleimp wesleimp left a comment

Choose a reason for hiding this comment

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

LGTM 👌

@gmlewis gmlewis merged commit e4b5117 into google:master Feb 28, 2020
@gmlewis gmlewis deleted the fix-1100 branch February 28, 2020 14:37
@joebowbeer
Copy link

@gmlewis Why was this breaking change in the API released in v29.0.3? I was expecting v29.0.3 to be interchangeable with v29.0.2

@gmlewis
Copy link
Collaborator Author

gmlewis commented Mar 13, 2020

I apologize... that was a slip on my part. Someone asked for a new release to be created and I forgot that breaking changes were included. I don't know how to fix it now, as it seems that once a release is made, it is archived in the mod tools.

To help this situation, I might be able to create a new v30.0.0 release now, but I'm afraid the damage has been done, and I'm sorry about that.

@joebowbeer
Copy link

My mistake: I don't think this was released to v29.0.3. It is one of the 17 commits since v29.0.3

@joebowbeer
Copy link

joebowbeer commented Mar 13, 2020

@gmlewis However, I am seeing the incompatible master src when I go get v29, which is causing inexplicable type errors to surface. A v30 release would be helpful, and would make it clear that master's go.mod is v30 source. Without an explicit v29 folder, I wonder if the v29 go.mod in the master branch is confusing the module system. Anyway, a v30 release would be appreciated!

@gmlewis
Copy link
Collaborator Author

gmlewis commented Mar 13, 2020

Alright, in that case I know for a fact that we have a large number of breaking changes between v29 and v30 and I believe one or two outstanding PRs will further break things. I was planning a v30 release would happen really soon in the hopes that the breaking changes would subside for a while (many APIs have been deprecated and changed lately from the v3 API)... so if we could hold off for just a few days, I think it would be the least disruptive for all users of the repo.

I'll try to prod along the outstanding PRs... but I can make no guarantees as to schedule, unfortunately.

@gmlewis
Copy link
Collaborator Author

gmlewis commented Mar 13, 2020

Taking a more detailed look, it appears that #1451 is the only one that is close that should get in before v30.0.0. I need a second LGTM on it, hopefully from @dmitshur but he probably wouldn't mind if another LGTM came in from someone else, since it appears to me that his comments have already been addressed.

@gmlewis
Copy link
Collaborator Author

gmlewis commented Mar 13, 2020

@joebowbeer - here's the plan - one more change is needed in #1451, then we can get #1456 approved by two LGTMs and merged, then we can release v30.0.0. Hopefully this can all happen today.

n1lesh pushed a commit to n1lesh/go-github that referenced this pull request Oct 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Indication that the PR author has signed a Google Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pointer in Issue.Labels missing?
4 participants