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.

Sign up for GitHub

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

Jump to bottom

R4R: Add goimports tool in makefile and format project #2767

Closed
wants to merge 7 commits into from
Closed

R4R: Add goimports tool in makefile and format project #2767

wants to merge 7 commits into from

Conversation

yutianwu
Copy link
Contributor

@yutianwu yutianwu commented Nov 12, 2018 •
edited
Loading

ref: #2766

  • Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
  • Wrote tests
  • Updated relevant documentation (docs/)
  • Added entries in PENDING.md with issue #
  • rereviewed Files changed in the github PR explorer

For Admin Use:

  • Added appropriate labels to PR (ex. wip, ready-for-review, docs)
  • Reviewers Assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@codecov
Copy link

codecov bot commented Nov 12, 2018 •
edited
Loading

Codecov Report

Merging #2767 into develop will decrease coverage by 0.16%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           develop    #2767      +/-   ##
===========================================
- Coverage    56.82%   56.65%   -0.17%     
===========================================
  Files          120      156      +36     
  Lines         8298     9783    +1485     
===========================================
+ Hits          4715     5543     +828     
- Misses        3265     3862     +597     
- Partials       318      378      +60

@yutianwu yutianwu mentioned this pull request Nov 12, 2018
Closed
alexanderbez
alexanderbez reviewed Nov 12, 2018
View reviewed changes
Makefile
@@ -208,6 +208,7 @@ test_lint:
format:
find . -name '*.go' -type f -not -path "./vendor*" -not -path "*.git*" -not -path "./client/lcd/statik/statik.go" | xargs gofmt -w -s
find . -name '*.go' -type f -not -path "./vendor*" -not -path "*.git*" -not -path "./client/lcd/statik/statik.go" | xargs misspell -w
find . -name '*.go' -type f -not -path "./vendor*" -not -path "*.git*" -not -path "./client/lcd/statik/statik.go" | xargs goimports -w -local github.com/cosmos/cosmos-sdk
Copy link
Contributor

Choose a reason for hiding this comment

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

Is goimports an automatically included binary with Go? I can't recall. If not, we'll have to make sure it's installed via the Makefile.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, we need to ensure that

alexanderbez
alexanderbez suggested changes Nov 12, 2018
View reviewed changes
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

Looks great! We just need need to make sure the goimports binary is installed.

@yutianwu
Copy link
Contributor Author

@alexanderbez add the dependency in get_tools, because the url of the tool is different, so I just put the line in Makefile.

alexanderbez
alexanderbez reviewed Nov 13, 2018
View reviewed changes
Makefile
@@ -116,6 +117,7 @@ update_dev_tools:
get_tools:
@echo "--> Installing tools"
$(MAKE) -C scripts get_tools
go get -u golang.org/x/tools/cmd/goimports
Copy link
Contributor

Choose a reason for hiding this comment

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

@ValarDragon @greg-szabo can we get this binary via the Makefile in /scripts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we can do that if we change the format of command getting tools

@rigelrozanski
Copy link
Contributor

merge conflicts need to be resolved before merge here

@yutianwu
Copy link
Contributor Author

merge conflicts need to be resolved before merge here

fixed

@greg-szabo
Copy link
Member

greg-szabo commented Nov 22, 2018 •
edited
Loading

(Apologies, I just found this PR, I don't know why I didn't see it earlier.)

I would split this into two PRs:

  1. The change in the Makefile.
  2. The results of the run.

The first one is something we need to thoroughly review, the second one is changing a lot of files in a predictable way. (Still worth reviewing but I feel it's easier.)

Regarding the first one, we have to make sure people use Makefiles right - the way we expect it to see. I can write a documentation on it and do a knowledge share with the team.

I need to open a new PR if you want me to fix it.

@yutianwu
Copy link
Contributor Author

yutianwu commented Nov 22, 2018 •
edited
Loading

(Apologies, I just found this PR, I don't know why I didn't see it earlier.)

I would split this into two PRs:

  1. The change in the Makefile.
  2. The results of the run.

The first one is something we need to thoroughly review, the second one is changing a lot of files in a predictable way. (Still worth reviewing but I feel it's easier.)

Regarding the first one, we have to make sure people use Makefiles right - the way we expect it to see. I can write a documentation on it and do a knowledge share with the team.

I need to open a new PR if you want me to fix it.

yeah, I am working on to change the Makefile, but it's different from other tools. if you are available, you can create a new one:) kinda busy lately

it seems it will cause lint warning using goimports...

@greg-szabo
Copy link
Member

I opened up the scripts/Makefile to see what it requires to implement this the right way.
Do you want to lock this tool down to a specific version, as we did with others?

Option 1: No need to lock it down to a specific version: we can just put go get in there, it should be Windows-compatible.
Option 2: We need to lock it down to a specific version: I need to rewrite the go_get target to take the source repository into account. Currently, only github.com is covered, but this tool lives in gopkg.org.

@greg-szabo greg-szabo mentioned this pull request Nov 22, 2018
Merged
6 tasks
@greg-szabo
Copy link
Member

Ok, so I've opened a new PR #2889 that details the change to

  • Add goimports as a tool
  • Include the goimports execution in the format target.

@yutianwu please check that PR, I can't possibly go through all 212 files changed on this one to see I didn't miss anything you were trying to do. We can open a separate PR that will just be the result of the make format command. This was the mechanics and the outcome is separated.

If everyone agrees with this approach, then this PR should be closed without merge.

@jackzampolin
Copy link
Member

@greg-szabo I really like this approach. Lets move the work over to your PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jackzampolin jackzampolin jackzampolin left review comments

@alexanderbez alexanderbez alexanderbez requested changes

@cwgoes cwgoes Awaiting requested review from cwgoes

@ebuchman ebuchman Awaiting requested review from ebuchman

@rigelrozanski rigelrozanski Awaiting requested review from rigelrozanski

@zramsay zramsay Awaiting requested review from zramsay

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@yutianwu @rigelrozanski @greg-szabo @jackzampolin @alexanderbez