-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Setup lint and format of Go, Proto and yaml/markdown #223
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
Conversation
acls.go
Outdated
func (h *Headscale) generateACLPolicyDestPorts(d string) ([]tailcfg.NetPortRange, error) { | ||
func (h *Headscale) generateACLPolicyDestPorts( | ||
d string, | ||
) ([]tailcfg.NetPortRange, error) { | ||
tokens := strings.Split(d, ":") | ||
if len(tokens) < 2 || len(tokens) > 3 { | ||
return nil, errorInvalidPortFormat |
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.
Is this the standard gofmt config?
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 mean in general, not just this line...
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.
No, its gofumpt,which I think is standard in vscode, and then it is run through golines to break long lines.
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.
Added note in readme.
|
||
fmt: | ||
prettier --check '**/**.{ts,js,md,yaml,yml,sass,css,scss,html}' | ||
golines --max-len=88 --base-formatter=gofumpt -w $(GO_SOURCES) |
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.
Why 88?
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.
It is based on python black (and Raymond Hettinger), which some might hate.
The idea is to make the code more naturally fit on narrow screens, so you can have multiple files open without bad editor wrapping.
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'll adda note in the reader about code style
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.
Done
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.
All good!
This is ready for now. This should give us a lot of helpers to keep consistent good practice. |
PTAL @juanfont, I think we need to have the old lint requirements removed |
@@ -49,3 +46,15 @@ linters: | |||
- cyclop | |||
- nestif | |||
- wsl # might be incompatible with gofumpt | |||
|
|||
linters-settings: | |||
varnamelen: |
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 will hurt
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 have fixed The part that hurts
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.
Wow. Just, wow.
This PR sets up linting for all our different file types in the CI and adds a format command to make to try to improve consistency.
I have disabled a bunch of linters, some intentionally.
However there is a list of "best practice" linters that I think we should add, but we have a solid backlog to get them passing.
This PR is initial work for #219