Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

Use goimports instead of gofmt #214

Merged

Conversation

VijayanB
Copy link
Member

goimports fixes imports order and also formats like gofmt.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@codecov
Copy link

codecov bot commented Aug 20, 2020

Codecov Report

Merging #214 into master will decrease coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #214      +/-   ##
============================================
- Coverage     72.40%   72.36%   -0.04%     
+ Complexity     1289     1288       -1     
============================================
  Files           139      139              
  Lines          6073     6073              
  Branches        469      469              
============================================
- Hits           4397     4395       -2     
- Misses         1464     1465       +1     
- Partials        212      213       +1     
Flag Coverage Δ Complexity Δ
#cli 80.30% <ø> (ø) 0.00 <ø> (ø)
#plugin 71.30% <ø> (-0.04%) 1288.00 <ø> (-1.00)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ Complexity Δ
cli/internal/controller/ad/ad.go 77.38% <ø> (ø) 0.00 <0.00> (ø)
cli/internal/controller/es/es.go 100.00% <ø> (ø) 0.00 <0.00> (ø)
...asticsearch/ad/cluster/ADClusterEventListener.java 88.00% <0.00%> (-4.00%) 13.00% <0.00%> (-1.00%)

@VijayanB VijayanB requested review from wnbts, kaituo and ylwu-amzn August 20, 2020 20:11
goimports fixes imports order and also formats like gofmt.
@VijayanB VijayanB self-assigned this Aug 20, 2020
@VijayanB
Copy link
Member Author

No functional change in this commit. I just formatted using goimports tool instead of gofmt tool.

@@ -39,18 +39,20 @@ jobs:
- name: Check out AD CLI
uses: actions/checkout@v2

- name: gofmt
run: gofmt -s -w .
- name: Format
Copy link
Member

Choose a reason for hiding this comment

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

After format, where is the formatted code located? I thought the workflow machine have the formatted code. How does it connect with the pull request and the master branch code?

Copy link
Member Author

Choose a reason for hiding this comment

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

go provides tool to format source code. We have to manually run this tool on source code to format before merging the code. Developers can forget to format . This check will run the format and update the code if it is not run by the developer. In next step, i have check whether working directory is dirty or not, if so then developer forgot to run and this will return with error.

@kaituo
Copy link
Member

kaituo commented Aug 21, 2020

No functional change in this commit. I just formatted using goimports tool instead of gofmt tool.

So you format the code manually or by workflow?

@VijayanB
Copy link
Member Author

No functional change in this commit. I just formatted using goimports tool instead of gofmt tool.

So you format the code manually or by workflow?

We have to format manually by using this tool ( goimports). Workflow will check if formatting is not done by developer, it will exit with error.

@VijayanB VijayanB requested a review from kaituo August 21, 2020 18:03
@VijayanB VijayanB merged commit 942a10a into opendistro-for-elasticsearch:master Aug 24, 2020
@VijayanB VijayanB deleted the format-goimport branch August 24, 2020 20:58
@VijayanB VijayanB added the infra label Aug 25, 2020
@yizheliu-amazon yizheliu-amazon mentioned this pull request Aug 27, 2020
yizheliu-amazon pushed a commit that referenced this pull request Aug 28, 2020
goimports fixes imports order and also formats like gofmt.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants