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

fix: Small fix gofmt and misspell from go report #253

Closed
wants to merge 1 commit into from
Closed

fix: Small fix gofmt and misspell from go report #253

wants to merge 1 commit into from

Conversation

Bablzz
Copy link
Contributor

@Bablzz Bablzz commented Oct 2, 2020

Hi there!

I've looked at goreportcard and in my mind it's not to be bad fix a few warn from that website.

If you think that it is not good I am going to close this MR.

Have a great day!

@gianarb
Copy link
Member

gianarb commented Oct 2, 2020

Hello @Bablzz thank you for your contribution. The typo is appreciated, the other one does not really add value to me. But it is good to embrace standards because it helps maintainability. But as I said for #251 those standards have to be checked continuously in CI. I never used goreportcard but in another project, I use golangci-lint. I am not a fan of those or better, I don't have any preference.

@mdelapenya do you have a preference in terms of the static analyzer for Go?

@gianarb gianarb added the chore Changes that do not impact the existing functionality label Oct 2, 2020
@mdelapenya
Copy link
Member

mdelapenya commented Oct 2, 2020

I've used golangci-lint, and it works fairly simple. E can enforce it in a pre-commit stage/hook (see https://pre-commit.com)

Sorry for the noise with the closer/reopen, using the phone is not always a good choice.

@mdelapenya mdelapenya closed this Oct 2, 2020
@mdelapenya mdelapenya reopened this Oct 2, 2020
@Bablzz
Copy link
Contributor Author

Bablzz commented Oct 2, 2020

Maybe use golanglint-ci in travis ci, but now this tool shows a few issue which some useful some not.

@gianarb
Copy link
Member

gianarb commented Oct 2, 2020

Yeah, personally I don't want to merge this PR because it does not add any value. I am happy to merge one that fixes the typos. I am not that excited about introducing a static check. And I don't want to change what I can't continuously assert. It will just be another thing to look during code review.

Thanks

@gianarb gianarb closed this Oct 2, 2020
@Bablzz
Copy link
Contributor Author

Bablzz commented Oct 2, 2020

if you don't mind, I will fix the typos in another MR

@Bablzz Bablzz mentioned this pull request Oct 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Changes that do not impact the existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants