Skip to content
This repository has been archived by the owner on May 11, 2024. It is now read-only.

feat(ci): add goimports sorting lint #734

Closed
wants to merge 1 commit into from
Closed

feat(ci): add goimports sorting lint #734

wants to merge 1 commit into from

Conversation

ronething
Copy link

@ronething ronething commented Apr 20, 2024

@mask-pp
Copy link
Contributor

mask-pp commented Apr 21, 2024

Sorry, our formatting does need to be improved, but I don't agree with this PR that overly complicates this function. I think several commands should be able to do it directly.

@mask-pp mask-pp closed this Apr 21, 2024
@ronething
Copy link
Author

ronething commented Apr 21, 2024

@mask-pp Hi, thanks for you reply.

but I don't agree with this PR that overly complicates this function.

But I'd like to know what the complexities are. The main changes are in the test.yaml and the sort import script file. Other Go files have been automatically modified by executing make sort-import.

The reason for adding the sort import lint to test.yaml is to ensure that everyone can sort import go files before PR merged.

ref:

I think several commands should be able to do it directly.

what's your option, only sort import go files and add command to Makefile?

@mask-pp
Copy link
Contributor

mask-pp commented Apr 21, 2024

@mask-pp Hi, thanks for you reply.

but I don't agree with this PR that overly complicates this function.

But I'd like to know what the complexities are. The main changes are in the test.yaml and the sort import script file. Other Go files have been automatically modified by executing make sort-import.

The reason for adding the sort import lint to test.yaml is to ensure that everyone can sort import go files before PR merged.

ref:

I think several commands should be able to do it directly.

what's your option, only sort import go files and add command to Makefile?

Cmd goimports -local "github.com/taikoxyz/taiko-client" -w ./ is enough.

@ronething
Copy link
Author

ronething commented Apr 21, 2024

Cmd goimports -local "github.com/taikoxyz/taiko-client" -w ./ is enough.

@mask-pp Maybe not, i use goimports -local "github.com/taikoxyz/taiko-client" -w ./, and prover/event_handler/block_proposed_test.go result is like below.

In fact, github.com/taikoxyz/taiko-client should need the same group here, but it just adds a blank line.

image

and i see the same result in your PR. https://github.com/taikoxyz/taiko-client/pull/740/files#diff-e985b5d04a4e6999752689f555f94f1cafc80b71f7f34df5f9c98712f7940234

if we use https://github.com/incu6us/goimports-reviser, it will have the right result.

ref: https://github.com/taikoxyz/taiko-client/pull/734/files#diff-e985b5d04a4e6999752689f555f94f1cafc80b71f7f34df5f9c98712f7940234

If there is no problem as I described, can you consider reopening this PR, thanks

cc @davidtaikocha

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants