Skip to content

Enforce no golangci-lint warnings in CI#313

Merged
sbarzowski merged 2 commits intogoogle:masterfrom
sbarzowski:golangci-lint
Mar 5, 2020
Merged

Enforce no golangci-lint warnings in CI#313
sbarzowski merged 2 commits intogoogle:masterfrom
sbarzowski:golangci-lint

Conversation

@sbarzowski
Copy link
Copy Markdown
Contributor

@sbarzowski sbarzowski commented Sep 1, 2019

No description provided.

@sbarzowski sbarzowski changed the title Enforce no golangci-lint warnings in CI [Do not merge] Enforce no golangci-lint warnings in CI Sep 1, 2019
@sbarzowski
Copy link
Copy Markdown
Contributor Author

sbarzowski commented Sep 1, 2019

It can use all golint warnings + quite a few others. Most importantly for CI purposes, the warnings can be silenced (and sometimes it's necessary for backward compatibility).

@sbarzowski sbarzowski mentioned this pull request Sep 1, 2019
Closed
@sbarzowski sbarzowski force-pushed the golangci-lint branch 3 times, most recently from 4e7c65a to 80afc66 Compare September 1, 2019 18:20
@sbarzowski
Copy link
Copy Markdown
Contributor Author

sbarzowski commented Sep 1, 2019

Of course the warnings will need to be fixed (or silenced) before merging this.

@sbarzowski
Copy link
Copy Markdown
Contributor Author

sbarzowski commented Sep 1, 2019

Unfortunately this change requires bumping the supported version of Go to 1.11. This is actually not that bad, since that's the oldest version officially supported by the Go project since Feb 2019.

@ghostsquad
Copy link
Copy Markdown
Contributor

ghostsquad commented Jan 26, 2020

@sbarzowski I think this can be easily merged now that minimum go version is 1.11

@sbarzowski sbarzowski changed the title [Do not merge] Enforce no golangci-lint warnings in CI Enforce no golangci-lint warnings in CI Mar 1, 2020
sbarzowski
sbarzowski commented Mar 1, 2020
View reviewed changes
internal/parser/lexer.go
for _, l := range lines {
if l[0] == '*' {
l = " " + l
for i := range lines {
Copy link
Copy Markdown
Contributor Author

@sbarzowski sbarzowski Mar 1, 2020

Choose a reason for hiding this comment

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

@sparkprime I don't really understand the point of this. Why do we need this "little hack"?

FYI before my change this whole loop was a noop, because l was a copy.

sbarzowski
sbarzowski commented Mar 1, 2020
View reviewed changes
builtins.go

data.Sort()
err = data.Sort()
if err != nil {
Copy link
Copy Markdown
Contributor Author

@sbarzowski sbarzowski Mar 1, 2020

Choose a reason for hiding this comment

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

This is probably the most consequential part of this change. In particular one of our tests already covers this, but I have missed it before (it was committed with a bad golden).

@coveralls
Copy link
Copy Markdown

coveralls commented Mar 1, 2020

Coverage Status

Coverage increased (+0.09%) to 78.589% when pulling 8dfb921 on sbarzowski:golangci-lint into 8b2b1ba on google:master.

@coveralls
Copy link
Copy Markdown

coveralls commented Mar 1, 2020 •
edited
Loading

Coverage Status

Coverage increased (+0.2%) to 78.7% when pulling da35d37 on sbarzowski:golangci-lint into 803ad64 on google:master.

@sbarzowski
Make golangci-lint happy
d671c89 
Some of the suggestions are minor bug fixes (missing error handling).
@sbarzowski
Enforce no golangci-lint warnings in CI
da35d37
@sbarzowski sbarzowski merged commit 234b97c into google:master Mar 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

cla: yes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@sbarzowski @ghostsquad @coveralls @googlebot