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

chore: fix Makefile for format and execute make format #230

Merged
merged 15 commits into from
Sep 14, 2023

Conversation

loloicci
Copy link
Contributor

@loloicci loloicci commented Jul 4, 2023

Description

This PR fixes make format command and executes it.

This PR replaces the lint tool for importing orders with goimports-reviser (https://github.com/incu6us/goimports-reviser). This makes it unify how to order imports regardless of how programmers separate imports into some blocks.

Motivation and context

Without this PR, make format formats client/docs/statik/statik.go and it causes a CI error.

Without this PR, how to separate format blocks is on how programmers write them.

How has this been tested?

Not needed.

Checklist:

  • I followed the contributing guidelines and code of conduct.
  • I have added a relevant changelog to CHANGELOG.md
  • I have added tests to cover my changes. (not needed)
  • I have updated the documentation accordingly. (not needed)

@loloicci loloicci self-assigned this Jul 4, 2023
@loloicci loloicci requested a review from zemyblue July 4, 2023 05:57
@codecov
Copy link

codecov bot commented Jul 4, 2023

Codecov Report

Merging #230 (2773527) into main (880ef9d) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #230   +/-   ##
=======================================
  Coverage   44.94%   44.94%           
=======================================
  Files           8        8           
  Lines        1068     1068           
=======================================
  Hits          480      480           
  Misses        570      570           
  Partials       18       18           
Files Changed Coverage Δ
ante/ante.go 0.00% <ø> (ø)
app/app.go 84.12% <ø> (ø)
cmd/fnsad/cmd/root.go 12.91% <ø> (ø)

Copy link
Member

@zemyblue zemyblue left a comment

Choose a reason for hiding this comment

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

To use make format, we need to install goimports. So how about adding golang.org/x/tools in go.mod as using ggo install golang.org/x/tools/cmd/goimports?

@loloicci
Copy link
Contributor Author

To use make format, we need to install goimports. So how about adding golang.org/x/tools in go.mod as using ggo install golang.org/x/tools/cmd/goimports?

I think it is not needed because goimports is not needed to build or install fnsa.
And, I want to leave it as another issue if needed. This is not in this PR's scope.

@zemyblue
Copy link
Member

zemyblue commented Jul 12, 2023

To use make format, we need to install goimports. So how about adding golang.org/x/tools in go.mod as using ggo install golang.org/x/tools/cmd/goimports?

I think it is not needed because goimports is not needed to build or install fnsa. And, I want to leave it as another issue if needed. This is not in this PR's scope.

Ok, I understand. Thank you.

Copy link
Member

@zemyblue zemyblue left a comment

Choose a reason for hiding this comment

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

I had mistaken at #221. We can fix this bug by adding "" to the next line.

-X github.com/Finschia/finschia-sdk/version.BuildTags=$(build_tags_comma_sep) \

This is a rollback in this changes.
https://github.com/Finschia/finschia/pull/221/files#diff-76ed074a9305c04054cdebb9e9aad2d818052b07091de1f20cad0bbac34ffb52L118-R119

Copy link
Member

@zemyblue zemyblue left a comment

Choose a reason for hiding this comment

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

Do you know the import path sorting rule?

@loloicci
Copy link
Contributor Author

Do you know the import path sorting rule?

It uses goimports, but I don't know in what order goimports sort imports, sorry.

@zemyblue
Copy link
Member

zemyblue commented Jul 13, 2023

Do you know the import path sorting rule?

It uses goimports, but I don't know in what order goimports sort imports, sorry.

I think this sorting rule is difference with ours. So this may make conflict with others.
Finschia/ostracon#559 (review)

How do you think about this?

@loloicci
Copy link
Contributor Author

loloicci commented Jul 18, 2023

Do you know the import path sorting rule?

It uses goimports, but I don't know in what order goimports sort imports, sorry.

I think this sorting rule is difference with ours. So this may make conflict with others.
Finschia/ostracon#559 (review)

How do you think about this?

How about replacing -local github.com/Finschia/finschia-sdk with -local github.com/Finschia

-local means

-local string
put imports beginning with this string after 3rd-party packages; comma-separated list

And, executed example is loloicci@6f2afa6

@loloicci loloicci force-pushed the make-format branch 3 times, most recently from 28fbb81 to cb1a11f Compare September 11, 2023 09:30
@loloicci loloicci force-pushed the make-format branch 4 times, most recently from d481494 to 532767f Compare September 11, 2023 09:44
@loloicci
Copy link
Contributor Author

I replaced the lint tool for import with goimports-reviser. This enables to unify import block format.

@loloicci loloicci removed the request for review from dudong2 September 12, 2023 01:45
app/app.go Show resolved Hide resolved
zemyblue
zemyblue previously approved these changes Sep 12, 2023
170210
170210 previously approved these changes Sep 13, 2023
@170210 170210 self-requested a review September 14, 2023 04:44
Copy link
Contributor

@170210 170210 left a comment

Choose a reason for hiding this comment

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

Could jobs in workflow be combined into one? And if you want to use cache for next time, it maybe need reverse the order of "install tools" and "actions/cache".

@loloicci loloicci dismissed stale reviews from 170210 and zemyblue via 5c80df7 September 14, 2023 05:23
@loloicci loloicci force-pushed the make-format branch 2 times, most recently from 0e72753 to 683da10 Compare September 14, 2023 05:27
Copy link
Contributor

@170210 170210 left a comment

Choose a reason for hiding this comment

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

LGTM

@loloicci loloicci merged commit 5a79efb into Finschia:main Sep 14, 2023
34 checks passed
@zemyblue zemyblue mentioned this pull request Nov 7, 2023
@zemyblue zemyblue mentioned this pull request Mar 4, 2024
@zemyblue zemyblue mentioned this pull request Jun 25, 2024
9 tasks
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