Skip to content

chore: make format the whole repo#1516

Closed
rach-id wants to merge 2 commits intov0.34.x-celestiafrom
fmt
Closed

chore: make format the whole repo#1516
rach-id wants to merge 2 commits intov0.34.x-celestiafrom
fmt

Conversation

@rach-id
Copy link
Copy Markdown
Member

@rach-id rach-id commented Oct 9, 2024

runs make format and make proto-format to format the whole repo.

This will make it easier to format next contributions without having to touch unrelated files.

@rach-id rach-id added the T:chore label Oct 9, 2024
@rach-id rach-id self-assigned this Oct 9, 2024
@rach-id rach-id requested a review from a team as a code owner October 9, 2024 08:29
@rach-id rach-id requested review from evan-forbes and rootulp and removed request for a team October 9, 2024 08:29
rootulp
rootulp previously approved these changes Oct 9, 2024
Copy link
Copy Markdown
Collaborator

@rootulp rootulp left a comment

Choose a reason for hiding this comment

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

LGTM w/ one optional idea

Comment on lines 15 to +17

dbm "github.com/cometbft/cometbft-db"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[observation] make format runs:

	find . -name '*.go' -type f -not -path "*.git*" -not -name '*.pb.go' -not -name '*pb_test.go' | xargs gofmt -w -s
	find . -name '*.go' -type f -not -path "*.git*"  -not -name '*.pb.go' -not -name '*pb_test.go' | xargs goimports -w -local github.com/cometbft/cometbft

even though golangci.yml doesn't have any rules to special case the order of imports for github.com/cometbft/cometbft. In other words, this PR seems fine to me but a future contributor can accidentally modify the order of imports and we won't get signal on it.

IMO the order of imports isn't important so I propose we modify the Makefile to do this:

fmt:
	@echo "--> Running golangci-lint --fix"
	@golangci-lint run --fix

and then re-run make format to fix the issues that golangci.yml has identified and can fix.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Member

@evan-forbes evan-forbes left a comment

Choose a reason for hiding this comment

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

weird shouldn't the lint ci block on these?

@rach-id
Copy link
Copy Markdown
Member Author

rach-id commented Oct 23, 2024

yes, its weird it doesn't. want me to investigate?

@evan-forbes
Copy link
Copy Markdown
Member

closing as this is targeting v0.34 but I wouldn't be surprised if we could do the same for v0.38

@evan-forbes evan-forbes closed this May 7, 2025
@github-project-automation github-project-automation Bot moved this to Done in core/app May 7, 2025
@rach-id rach-id deleted the fmt branch July 7, 2025 11:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants