Skip to content

add gofmt step to ci#91

Merged
gavv merged 4 commits into
roc-streaming:mainfrom
wataru-nakanishi:add_gofmt
May 21, 2023
Merged

add gofmt step to ci#91
gavv merged 4 commits into
roc-streaming:mainfrom
wataru-nakanishi:add_gofmt

Conversation

@wataru-nakanishi

@wataru-nakanishi wataru-nakanishi commented May 20, 2023

Copy link
Copy Markdown
Contributor

PR for #86

Implemented

  • Add gofmt CI step

This is my first contributing!
If the contributing process was wrong, please tell me.

@coveralls

coveralls commented May 20, 2023

Copy link
Copy Markdown
Collaborator

Coverage Status

Coverage: 94.04%. Remained the same when pulling 69f2983 on wataru-nakanishi:add_gofmt into 9647d5d on roc-streaming:main.

@wataru-nakanishi

Copy link
Copy Markdown
Contributor Author

@roc-streaming/go ready for review

Comment thread .github/workflows/build.yaml Outdated
with:
go-version: ${{ matrix.go }}

- name: gofmt

@wataru-nakanishi wataru-nakanishi May 20, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I added fmt step only in linux.
Because Jerome1337/gofmt-action@v1.0.5 is available in linux only.

https://github.com/roc-streaming/roc-go/actions/runs/5029732462/jobs/9021655456
Error: Container action is only supported on Linux

@gavv

gavv commented May 20, 2023

Copy link
Copy Markdown
Member

Thanks for PR! Using linux is perfectly ok, we don't need to check formatting on each OS because results would be the same.

I suggest extracting formatting check into a separate job "Formatting". It's not really bound to any steps in "Linux" job and can be done separately.

@gavv gavv added the status: needs revision Pull request should be revised by its author label May 20, 2023
@gavv

gavv commented May 20, 2023

Copy link
Copy Markdown
Member

Follow-up issue: https://github.com/roc-streaming/roc-go/issues/92

@gavv

gavv commented May 20, 2023

Copy link
Copy Markdown
Member

@wataru-nakanishi Could you please check if this CI step will fail if a file has no newline at the end? If it won't, I think I'll create a separate issue for adding such step too.

@wataru-nakanishi

wataru-nakanishi commented May 21, 2023

Copy link
Copy Markdown
Contributor Author

@gavv
Thank you!
Fixed. ec6806c

Could you please check if this CI step will fail if a file has no newline at the end? If it won't, I think I'll create a separate issue for adding such step too.

I added a new line to config.go and CI failed.
Originally, there is no new line
https://github.com/roc-streaming/roc-go/actions/runs/5035614704/jobs/9031245933?pr=91

@gavv

gavv commented May 21, 2023

Copy link
Copy Markdown
Member

I added a new line to config.go and CI failed.
Originally, there is no new line

I actually mean the opposite case: not when extra newline is added, but when the last one is missing.

github-diff-no-newline-warning.png

@wataru-nakanishi

Copy link
Copy Markdown
Contributor Author

@gavv
CI check didn't fail if a file has no newline at the end.
For example, config.go has no new line.
https://github.com/roc-streaming/roc-go/blob/main/roc/config.go#L494

Please let me know if I'm misunderstanding.

@gavv

gavv commented May 21, 2023

Copy link
Copy Markdown
Member

config.go does have newline at the end. It's last symbol is LF, not "}". If there were no terimnating LF, github would display that red icon that I showed above.

@wataru-nakanishi

Copy link
Copy Markdown
Contributor Author

@gavv

config.go does have newline at the end. It's last symbol is LF, not "}". If there were no terimnating LF, github would display that red icon that I showed above.

Thanks for letting me know.
CI failed if a file has no new line.
https://github.com/roc-streaming/roc-go/actions/runs/5035923474/jobs/9031753001?pr=91

@gavv

gavv commented May 21, 2023

Copy link
Copy Markdown
Member

Thanks!! So we don't need one more check, gofmt already checks everything we need.

One more comment: could you please place formatting job before release job, and add formatting job to the list of dependencies of release job?

@wataru-nakanishi

wataru-nakanishi commented May 21, 2023

Copy link
Copy Markdown
Contributor Author

@gavv

could you please place formatting job before release job, and add formatting job to the list of dependencies of release job?

fixed! 69f2983

@gavv gavv merged commit da75034 into roc-streaming:main May 21, 2023
@gavv

gavv commented May 21, 2023

Copy link
Copy Markdown
Member

LGTM

@gavv gavv removed the status: needs revision Pull request should be revised by its author label May 21, 2023
@wataru-nakanishi wataru-nakanishi deleted the add_gofmt branch May 21, 2023 14:56
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