-
Notifications
You must be signed in to change notification settings - Fork 27
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
Run additional linters as pre-commit hooks #1798
base: main
Are you sure you want to change the base?
Conversation
@jhiemstrawisc Assigning to you to find a reviewer. (I'll rebase this once 7.12 is out the door.) |
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.
Things here look okay to me and I wouldn't mind merging at is, although I had a few comments/questions I'm hoping you can address before we do that.
Changes notwithstanding, once this is rebased it should be okay to merge. Thanks Brian!
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.
Since this is a critical section of bash scripting that goes beyond testing, can you double check the changes to make sure we think they're okay?
@@ -358,7 +358,7 @@ func TestHandlePacket(t *testing.T) { | |||
expectedLinkConnectIncDup := strings.NewReader(mockPromLinkConnectInc) | |||
expectedLinkByteXferIncDup := strings.NewReader(mockPromLinkByteXferInc) | |||
|
|||
// First time received a summmary packet |
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.
And here I had assumed this was just an extra tasty packet!
Despite the original motivation for #1183, it turns out that most of the changes in this PR are to Go files. Also, while I'm here, I decided to add ShellCheck as another linter.
My intent behind the new configuration in
.pre-commit-config.yaml
and_typos.yaml
is to reduce the amount of code that needs "fixing" in this PR. Developers are always welcome to runtypos
andshellcheck
on their own, but given that this is an existing codebase, I don't think it's useful to flag every potential issue.Note that
typos
does not run a spell check but instead has a built-in database of common spelling corrections. As a result, there might still be spelling mistakes in the code base. Furthermore, while I've applied the changes suggested bytypos
, I didn't put much effort into reviewing every modified comment for clarity and grammar.I did confirm that
typos
seems to be willing to run on the vast majority of relevant files in this repository.For reviewers:
images/entrypoint.sh
that I'm guessing has never been used in the wild.