WIP: Make a reciever of FlavorResourceQuantites.For pointer

[tenzen-y]

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

For the FlavorResourceQuantities, Add uses the pointer receiver, For uses the non pointer receiver.
But, it would be better to use either a pointer or nonpointer receiver across all functions for the struct.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: tenzen-y

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [tenzen-y]

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

[netlify]

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name	Link
🔨 Latest commit	004ad6c
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/66a209eaa6bb960008e67ca6

[gabesaba]

Is this an improvement? Since FlavorResourceQuantities is a map type, I don't think we gain anything by using a pointer, and the new code is a less clean - 3 lines instead of 1, and have to deref inside ()

The only reason we used pointer receiver in the Add method is so that it could handle the null map case

[tenzen-y]

Is this an improvement? Since FlavorResourceQuantities is a map type, I don't think we gain anything by using a pointer, and the new code is a less clean - 3 lines instead of 1, and have to deref inside ()

That sounds reasonable.

The only reason we used pointer receiver in the Add method is so that it could handle the null map case

If so, shouldn't we use the non pointer f for Add function like the following since map type already pointer type, right?

func (f FlavorResourceQuantities) Add(fr FlavorResource, v int64) {
	if f == nil {
		f = make(FlavorResourceQuantities)
	}
...

[tenzen-y]

Is this an improvement? Since FlavorResourceQuantities is a map type, I don't think we gain anything by using a pointer, and the new code is a less clean - 3 lines instead of 1, and have to deref inside ()

That sounds reasonable.

The only reason we used pointer receiver in the Add method is so that it could handle the null map case

If so, shouldn't we use the non pointer f for Add function like the following since map type already pointer type, right?

func (f FlavorResourceQuantities) Add(fr FlavorResource, v int64) {
	if f == nil {
		f = make(FlavorResourceQuantities)
	}
...

Oh, I found the above code doesn't work well. NVM.

[trasc]

Is this an improvement? Since FlavorResourceQuantities is a map type, I don't think we gain anything by using a pointer, and the new code is a less clean - 3 lines instead of 1, and have to deref inside ()

That sounds reasonable.

The only reason we used pointer receiver in the Add method is so that it could handle the null map case

If so, shouldn't we use the non pointer f for Add function like the following since map type already pointer type, right?

func (f FlavorResourceQuantities) Add(fr FlavorResource, v int64) {
	if f == nil {
		f = make(FlavorResourceQuantities)
	}
...

You only change the local f , not the caller.

we can replace the Add method with something like append, and.

f = Append(f, k, v)

[k8s-ci-robot]

@tenzen-y: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kueue-test-unit-main	004ad6c	link	true	/test pull-kueue-test-unit-main

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

[tenzen-y]

Is this an improvement? Since FlavorResourceQuantities is a map type, I don't think we gain anything by using a pointer, and the new code is a less clean - 3 lines instead of 1, and have to deref inside ()

That sounds reasonable.

The only reason we used pointer receiver in the Add method is so that it could handle the null map case

If so, shouldn't we use the non pointer f for Add function like the following since map type already pointer type, right?

func (f FlavorResourceQuantities) Add(fr FlavorResource, v int64) {
	if f == nil {
		f = make(FlavorResourceQuantities)
	}
...

You only change the local f , not the caller.

Yeah, that's right. The reason why failure.

we can replace the Add method with something like append, and.

f = Append(f, k, v)

My motivation was to consistently use either a pointer or non-pointer receiver across all functions for the same struct and avoid the pointer map receiver.
Passing the f instance could work fine, but it may reduce its convenience of it compared with Add.

Let me think another better way...

[gabesaba]

Relevant code is deleted in #2721, so we can close this PR

[k8s-ci-robot]

PR needs rebase.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[mimowo]

/close
based on #2689 (comment)

[k8s-ci-robot]

@mimowo: Closed this PR.

Details

In response to this:

/close
based on #2689 (comment)

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.