-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
style: gofumpt and godot [skip changelog] #10081
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes me mostly happy. Thank you!
IMO it is worst to have inconsistent linters, it would be awesome if you would also submit it there https://github.com/ipfs/uci (this is a unified CI for our libraries across many repos). 🙂 |
@Jorropo yes, but they're not necessarily inconsistent. They're just stricter I would say. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this was in an acceptable state.
var ( | ||
wanPrefix = net.ParseIP("2001:218:3004::") | ||
lanPrefix = net.ParseIP("fe80::") | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we turn this one off ? This is not helping with readability and make copy-pastability worst and spread the code thin.
const apiFile = "api" | ||
const gatewayFile = "gateway" | ||
const swarmKeyFile = "swarm.key" | ||
const ( | ||
apiFile = "api" | ||
gatewayFile = "gateway" | ||
swarmKeyFile = "swarm.key" | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
similar for var, copy-pastability is worst when spread out.
type printFunc func(obj *routing.QueryEvent, out io.Writer, verbose bool) error | ||
type pfuncMap map[routing.QueryEventType]printFunc | ||
type ( | ||
printFunc func(obj *routing.QueryEvent, out io.Writer, verbose bool) error | ||
pfuncMap map[routing.QueryEventType]printFunc | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
similar to const and var
var ( | ||
errAllowOffline = errors.New("can't put while offline: pass `--allow-offline` to override") | ||
) | ||
var errAllowOffline = errors.New("can't put while offline: pass `--allow-offline` to override") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Altho I think this is up to judgment to use braces blocks for declaration, I think this is good, if you have only one thing don't put it in a brace block.
Actually I would like a linter that always don't use the braced declarations since they aren't easily copy pastable. |
Fixed comment in #10082 Re: copy-pasteability and new linter in UCI: those are opinions and most of our code already uses the braces for multiple variables. Updating the linter the UCI will create a cascade effect of many repositories to update. Likely not a priority. |
It's not an opinion, my way is a simple key press to cut the line, multi line is not as it often require putting the braces back correctly in the destination. The opinion is weather or not this matters but saying it does not is like arguing editing code does not matters. |
Well I have a fork of gofumpt now #10083 |
PS: I thought this PR added gofumpt to the CI, this was just a one shot. Things will degrade overtime if it's not enforced in CI FYI. |
I added the
godot
andgofumpt
linters to make a more readable code.