-
Notifications
You must be signed in to change notification settings - Fork 4
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
Add lint report, clean up coverage report #37
Conversation
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is great, and the output is really nice now! thank you. Just a note on formatting, and the linter has some comments for you as well :)
.drone/cmd/ghcomment/main.go
Outdated
panic(err) | ||
} | ||
|
||
commentTypeIdentifier := flag.String("i", "", "String that identifies the comment type being submitted") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prefer verbose flag names so they aren't as cryptic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do!
.drone/cmd/ghcomment/main.go
Outdated
return v | ||
} | ||
|
||
func minimizeOutdatedComments( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did the go formatter produce this output? The Go standard is to just let the line be very long. If you don't run gofmt by default, please do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought I had it setup to run on every ctrl+s but I guess it might be doing a Goland-specific formatting instead - will do!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from googling around I think the goland formatting is a bit non-standard. If it seems to insist on this formatting, there seems to be a way to force it to use gofmt instead: https://stackoverflow.com/questions/47735678/goland-how-to-use-gofmt
I'm mostly trying to avoid a formatter war where prs keep reformatting the same code over and over
.drone/cmd/ghcomment/main.go
Outdated
|
||
for _, comment := range prComments { | ||
if comment.Author.Login == CommenterLogin && | ||
strings.Contains(comment.Body, commentTypeIdentifier) && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
again, this should just be a long line
This comment has been minimized.
This comment has been minimized.
still some lint complaints -- also in the transmog repo, the lint is run as part of an automated check that can block merging. Personally I like the method you've used better, but other people might prefer consistency with our other repos. (I think we've done enough on this CI/CD stuff and should change focus to the auth stuff) |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
If the lint report is empty, can we print a happy message instead of an empty twirl-down? 😆 |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Go coverage report: Click to expand.
Go lint report: No issues found. 😎 |
@ywwg I think I've got everything fixed up for this PR--let me know if I've missed anything! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks!
This PR adds a lint report on top of the existing coverage report as well as cleaning up the interaction a bit: