Skip to content

Conversation

kolyshkin
Copy link
Contributor

@kolyshkin kolyshkin commented Mar 29, 2025

This form is available since Go 1.22 (see https://tip.golang.org/ref/spec#For_range) and will probably be seen more and more in the new code.

Copy link
Contributor

openshift-ci bot commented Mar 29, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kolyshkin

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

Copy link
Collaborator

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

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

LGTM WRT correctness of the changes.

As for the style … Aesthetically, I’m not personally much of a fan.

for _ := range 10 to express “10 times”, sure. The full for i := range 10, has, to my taste, a bit too much implicit and hidden about the interval this operates over.

Ultimately, I think it might be easier to given in than to continue fight default linters’ settings over this. That is, alone, a good enough reason to merge this.

I’ll, at least, let someone else chime in, on the small chance that my dislike of this syntax were universally and strongly shared.

@kolyshkin
Copy link
Contributor Author

LGTM WRT correctness of the changes.

As for the style … Aesthetically, I’m not personally much of a fan.

for _ := range 10 to express “10 times”, sure.

By the way, this can (and should) be shortened to for range 10.

The full for i := range 10, has, to my taste, a bit too much implicit and hidden about the interval this operates over.

I'm with you on that. To me, for i := 0; i < 10; i++ is way easier to read than for i := range 10.

I guess though this is mostly because I'm so much used to reading (and writing) in the (old/classical) way, thus I'm just biased. Over time, as more and more new style code will appear, the perception should change.

Ultimately, I think it might be easier to given in than to continue fight default linters’ settings over this. That is, alone, a good enough reason to merge this.

One other reason is, we all have to adopt to this new style (as this is now standard), and this PR is an attempt to speed up this process.

I’ll, at least, let someone else chime in, on the small chance that my dislike of this syntax were universally and strongly shared.

Would also like to hear more opinions on this one.

This form is available since Go 1.22 (see
https://tip.golang.org/ref/spec#For_range) and will probably be seen
more and more in the new code.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@rhatdan
Copy link
Member

rhatdan commented Apr 1, 2025

I am fine with the new style.
/lgtm

@openshift-ci openshift-ci bot added the lgtm label Apr 1, 2025
@openshift-merge-bot openshift-merge-bot bot merged commit 6f4bc1f into containers:main Apr 1, 2025
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants