-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Go: Support 1.24 #18306
Go: Support 1.24 #18306
Conversation
9155891
to
36d073f
Compare
27c555d
to
a444a6a
Compare
bf7c52c
to
afe4554
Compare
go/extractor/extractor.go
Outdated
// `typeNameObj` can only be an alias in versions of Go | ||
// before 1.24. |
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.
Should this be above the condition? It might also be useful to include what the implications of this are
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.
I wasn't quite sure where it should go. I can move it a line higher if you think that's clearer. I'm also happy to rephrase it, but I wasn't sure how to put it clearly. Suggestions welcome. Basically, before 1.24 both defined types and named types will satisfy tp, ok := typeNameObj.Type().(*types.Named); ok
, and we want to exclude the alias types from going into this branch of the conditional (though I'm not sure if anything bad would happen - I'm just preserving the old behaviour). From 1.24 onwards, alias types will instead satisfy tp, ok := typeNameObj.Type().(*types.Alias); ok
, and will be dealt with by the second conditional below this.
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.
I have moved it and tried to make it clearer.
Allow release candidate versions.
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.
Proxying @mbg's approval (as he is the author, he can't approve)
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.
Looks good. CI failure is expected and should be resolved once the internal PR is merged.
No description provided.