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

style: gofumpt and godot [skip changelog] #10081

Merged
merged 3 commits into from
Aug 17, 2023
Merged

style: gofumpt and godot [skip changelog] #10081

merged 3 commits into from
Aug 17, 2023

Conversation

kehiy
Copy link
Contributor

@kehiy kehiy commented Aug 17, 2023

I added the godot and gofumpt linters to make a more readable code.

@kehiy kehiy requested review from lidel and a team as code owners August 17, 2023 11:30
@kehiy kehiy changed the title style: gofumpt and godot style: gofumpt and godot [skip changelog] Aug 17, 2023
cmd/ipfs/init.go Outdated Show resolved Hide resolved
cmd/ipfs/init.go Outdated Show resolved Hide resolved
Copy link
Member

@hacdias hacdias left a 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!

@hacdias hacdias merged commit f12b372 into ipfs:master Aug 17, 2023
18 checks passed
@Jorropo
Copy link
Contributor

Jorropo commented Aug 17, 2023

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). 🙂

@hacdias
Copy link
Member

hacdias commented Aug 17, 2023

@Jorropo yes, but they're not necessarily inconsistent. They're just stricter I would say. godot I like (didn't know, but I always hate seeing comments without dots in the end). gofumpt just seems stricter than gofmt. Indeed it would be great to run both.

@kehiy kehiy deleted the feature/linter branch August 17, 2023 12:10
Copy link
Contributor

@Jorropo Jorropo left a 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.

Comment on lines +54 to +57
var (
wanPrefix = net.ParseIP("2001:218:3004::")
lanPrefix = net.ParseIP("fe80::")
)
Copy link
Contributor

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.

repo/fsrepo/migrations/fetch.go Show resolved Hide resolved
Comment on lines -67 to +71
const apiFile = "api"
const gatewayFile = "gateway"
const swarmKeyFile = "swarm.key"
const (
apiFile = "api"
gatewayFile = "gateway"
swarmKeyFile = "swarm.key"
)
Copy link
Contributor

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.

p2p/p2p.go Show resolved Hide resolved
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
)
Copy link
Contributor

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")
Copy link
Contributor

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.

core/commands/dag/dag.go Show resolved Hide resolved
@Jorropo
Copy link
Contributor

Jorropo commented Aug 17, 2023

Actually I would like a linter that always don't use the braced declarations since they aren't easily copy pastable.

@hacdias
Copy link
Member

hacdias commented Aug 17, 2023

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.

@Jorropo
Copy link
Contributor

Jorropo commented Aug 17, 2023

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.

@Jorropo
Copy link
Contributor

Jorropo commented Aug 17, 2023

Well I have a fork of gofumpt now #10083

@Jorropo
Copy link
Contributor

Jorropo commented Aug 17, 2023

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.

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