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.

Sign up for GitHub

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

Jump to bottom

add generic methods for integer package #280

Closed
wants to merge 2 commits into from
Closed

add generic methods for integer package #280

wants to merge 2 commits into from

Conversation

logicalhan
Copy link
Member

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Generics were introduced in go 1.18, we're at 1.20 so we can start using them in the utils package.

Release note:

IntMax,IntMin,Int32Max,Int32Min,Int64Max,Int64Min are now deprecated, please use `Max` or `Min` instead.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 10, 2023
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 10, 2023
@logicalhan
Copy link
Member Author

/assign @thockin

@logicalhan
Copy link
Member Author

/retest

(I'm a bit confused by this test failure)

@liggitt
Copy link
Member

liggitt commented May 10, 2023

this may be the first time apidiff has run since #243 merged, and I'm not sure it is handling diffing of generic APIs correctly :-/

it was happy when the generic APIs were new, but looks like it fails to understand a no-op to them

@liggitt
Copy link
Member

liggitt commented May 10, 2023

looks like apidiff added generics support in https://go-review.googlesource.com/c/exp/+/411076 but I'm not sure if this is exposing an edge it didn't handle

@logicalhan
Copy link
Member Author

this may be the first time apidiff has run since #243 merged, and I'm not sure it is handling diffing of generic APIs correctly :-/

Ha, I was just looking at apidiff code, it must be a bug.

@logicalhan logicalhan force-pushed the generics branch 2 times, most recently from 983db93 to 5dbc265 Compare May 10, 2023 01:28
thockin
thockin reviewed May 16, 2023
View reviewed changes
integer/integer.go Outdated Show resolved Hide resolved
thockin
thockin reviewed May 19, 2023
View reviewed changes
Copy link
Member

@thockin thockin left a comment

Choose a reason for hiding this comment

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

Thanks!

/lgtm
/approve

/hold

if you want to add uint cases, else remove hold

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels May 19, 2023
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 31, 2023
@thockin thockin removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 31, 2023
@thockin
Copy link
Member

thockin commented May 31, 2023

Thanks!

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 31, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: logicalhan, thockin

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

MadhavJivrajani
MadhavJivrajani requested changes Jun 21, 2023
View reviewed changes
Copy link
Contributor

@MadhavJivrajani MadhavJivrajani left a comment

Choose a reason for hiding this comment

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

/hold

integer/integer.go
Comment on lines +20 to +22
type number interface {
~int | ~int8 | ~int16 | ~int32 | ~int64 | ~uint | ~uint8 | ~uint16 | ~uint32 | ~uint64 | ~uintptr
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Introducing a user-defined constraint here will continue breaking CI till golang/go#60911 is resolved.
See: #284

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 21, 2023
@MadhavJivrajani
Copy link
Contributor

/retest

@MadhavJivrajani
Copy link
Contributor

MadhavJivrajani commented Jul 11, 2023 •
edited

/retest
I'm not sure why the retrigger isn't working after #284
Will try to manually retrigger for now.

@MadhavJivrajani
Copy link
Contributor

@logicalhan -- I see that GH workflows can't be retriggered after 30 days of its initial run, I think you'll have to push an empty commit to retrigger a run here :/
The job is back on its feet fwiw.

@logicalhan
Copy link
Member Author

This PR is going to be obsolete actually due to https://tip.golang.org/ref/spec#Min_and_max.

@logicalhan logicalhan closed this Jul 11, 2023
@Priyankasaggu11929 Priyankasaggu11929 mentioned this pull request Jul 17, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thockin thockin thockin left review comments

@MadhavJivrajani MadhavJivrajani MadhavJivrajani requested changes

@andrewsykim andrewsykim Awaiting requested review from andrewsykim

@deads2k deads2k Awaiting requested review from deads2k

Assignees

@thockin thockin

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@logicalhan @liggitt @thockin @k8s-ci-robot @MadhavJivrajani