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.

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

change the for loop to a new for range - part 1 #4996

Closed
wants to merge 1 commit into from

Conversation

pfi79
Copy link
Contributor

@pfi79 pfi79 commented Sep 28, 2024

I suggest replacing the construction:
for i:= ...; i < ...; i++
by the new construction:
for range ...
or
for i := range ...

there are a lot of changes, but I'll break them down into a few pr

Signed-off-by: Fedor Partanskiy <fredprtnsk@gmail.com>
@pfi79 pfi79 requested a review from a team as a code owner September 28, 2024 13:30
@denyeart
Copy link
Contributor

Although Go 1.23 expanded where you can use range, that doesn't necessarily mean we should change every for loop throughout the existing code.

For simple counters I think the for loop is fine and maybe even more readable by most developers. Plus, changing code unnecessarily in the main branch will make backports more difficult. And then there is also risk of an "off by 1" mistake when making bulk changes in the code.

Is there a compelling reason to make the change? Maybe we should only update occurrences where there is a compelling need, or if we are updating a section of code regardless.

@pfi79
Copy link
Contributor Author

pfi79 commented Sep 30, 2024

Although Go 1.23 expanded where you can use range, that doesn't necessarily mean we should change every for loop throughout the existing code.

For simple counters I think the for loop is fine and maybe even more readable by most developers. Plus, changing code unnecessarily in the main branch will make backports more difficult. And then there is also risk of an "off by 1" mistake when making bulk changes in the code.

Is there a compelling reason to make the change? Maybe we should only update occurrences where there is a compelling need, or if we are updating a section of code regardless.

There's no good reason.

There is no uniformity in the for ; ;i++ construct.
That's why they invented a substitute for it, in which it is harder to make a mistake. So I decided to make it monotonous.

You may not accept my suggestion and just close it.

But if you decide to move in this direction, I can close this pr and create smaller ones to make it easier for the checkers to track down the “off by 1” error.

@denyeart
Copy link
Contributor

denyeart commented Oct 1, 2024

Although Go 1.23 expanded where you can use range, that doesn't necessarily mean we should change every for loop throughout the existing code.
For simple counters I think the for loop is fine and maybe even more readable by most developers. Plus, changing code unnecessarily in the main branch will make backports more difficult. And then there is also risk of an "off by 1" mistake when making bulk changes in the code.
Is there a compelling reason to make the change? Maybe we should only update occurrences where there is a compelling need, or if we are updating a section of code regardless.

There's no good reason.

There is no uniformity in the for ; ;i++ construct. That's why they invented a substitute for it, in which it is harder to make a mistake. So I decided to make it monotonous.

You may not accept my suggestion and just close it.

But if you decide to move in this direction, I can close this pr and create smaller ones to make it easier for the checkers to track down the “off by 1” error.

My opinion is to not make blanket changes in the code base for the purpose of syntax preferences. Different developers may have different preferences. If there are generally accepted bad Go practices in the code base then I agree those should be changed, but I don't think this one meets that criteria.

Let's keep it open for another day to see if there are any other thoughts from other maintainers.

@yacovm
Copy link
Contributor

yacovm commented Oct 1, 2024

Let's keep it open for another day to see if there are any other thoughts from other maintainers.

Yeah I second @denyeart .

If it ain't broke, don't fix it :-)

@denyeart denyeart closed this Oct 1, 2024
@pfi79 pfi79 deleted the for-range branch October 1, 2024 21:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants