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

Use go 1.22 language features. #488

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

jholdstock
Copy link
Member

Based on #487

Copy link
Member

@davecgh davecgh left a comment

Choose a reason for hiding this comment

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

Reads well.

This seems entirely pointless though. It needlessly requires anyone who might want to pull in the vpsd module to force their own consumers to a minimum of Go 1.22 for a feature that provides zero benefit.

@jholdstock
Copy link
Member Author

jholdstock commented Aug 15, 2024

Thats why I opened this as a separate PR, I am still in two minds about it.

IMO updating the for loops is worthwhile because it tidies up the code and removes the chance of off-by-one errors (and other bugs), but I'm not sure it merits a version bump on its own. Perhaps this PR can remain open as a TODO, to be revisited on the next go version bump.

@davecgh
Copy link
Member

davecgh commented Aug 15, 2024

IMO updating the for loops is worthwhile because it tidies up the code and removes the chance of off-by-one errors (and other bugs), but I'm not sure it merits a version bump on its own.

I guess I'm just too used to normal for loops, but I find them more obvious and less error prone than the new syntax myself.

When I see a range, it requires thinking about whether it's the index or the value (like in maps) which means looking at the type of the target of the range and remembering which inconsistent semantics are at play. Then, there is the whole change so that the loop variable is per-iteration scope instead of per-loop scope in Go 1.22 which means the go.mod version needs to be consulted when reviewing to know which behavior you're dealing with. Now, you also have the pleasure of consulting the go.mod version yet again to determine if the target can even be ranged over (for the new types).

On the other hand, for i := 0; i < num; i++ { is 100% obvious exactly what it's going to do with no additional context.

@jholdstock jholdstock mentioned this pull request Sep 20, 2024
@jholdstock
Copy link
Member Author

==> lint main module
WARN [linters_context] copyloopvar: this linter is disabled because the Go version (1.20) of your project is lower than Go 1.22 
==> lint client
WARN [linters_context] copyloopvar: this linter is disabled because the Go version (1.19) of your project is lower than Go 1.22 
==> lint types
WARN [linters_context] copyloopvar: this linter is disabled because the Go version (1.19) of your project is lower than Go 1.22 

Newly introduced copyloopvar linter does not run unless go.mod files are updated.

@jholdstock
Copy link
Member Author

Rebased onto #490 and including the linter updates from #489

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.

2 participants