Run gopls modernize on the codebase#33739
Conversation
|
It seems that at least one fix that was applied was unsafe. I suppose this is a bug in the module: Tool output: |
|
Given the "internal" nature of this thing, maybe we should just take this as a one-time refactoring action for now. Maybe in the future |
delvh
left a comment
There was a problem hiding this comment.
I agree with your assessment, it is fine as a one time action, but we should not add it to the CI while it is inofficial.
| // the config system may change the environment variables, so get a copy first, to be used later | ||
| env := append([]string{}, os.Environ()...) | ||
| env := os.Environ() |
There was a problem hiding this comment.
This change was also erroneous.
Although the code itself should probably use the maps.Clone function anyway…
There was a problem hiding this comment.
Actually no, I think the fix is correct. os.Environ() returns string[], so maps.Clone will not work.
I think it does not make sense to clone a string[] here, or do you? I think even if the environment changes after this call, the value will stay the same.
There was a problem hiding this comment.
It's not a gopls bug. os.Environ always returns a new slice, so no need to "clone"
| } else { | ||
| Mirror.DefaultInterval = time.Hour * 8 | ||
| } | ||
| Mirror.DefaultInterval = max(time.Hour*8, Mirror.MinInterval) |
There was a problem hiding this comment.
I agree with the tool, that's what the code is doing.
However, it's not what it should be doing:
I think the code is supposed to be
| Mirror.DefaultInterval = max(time.Hour*8, Mirror.MinInterval) | |
| Mirror.DefaultInterval = Mirror.MinInterval |
Comparing against the default value looks strange
There was a problem hiding this comment.
I hope to keep behaviour changes to a minimum in this PR.
There was a problem hiding this comment.
Yes, avoid unnecessary changes.
|
Okay, I have reverted all the tooling changes and added the command to run to the PR message above for future reference. |
modernize on the codebase
|
I think best course of action will be to report the bugs upstream to gopls and then re-run again here. Currently I don't have motivation to do that, so if someone can see the bugs introduced here, feel free to file gopls bugs. |
|
I'll revisit this at a later date when modernize has hopefully worked out their bugs, it seems to be in active development, so I expect its quality to get better eventually. |
gopls has a internal modernize code action which we can run on the CLI which I have done here: