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

Setup lint and format of Go, Proto and yaml/markdown #223

Merged
merged 41 commits into from
Nov 15, 2021

Conversation

kradalby
Copy link
Collaborator

@kradalby kradalby commented Nov 13, 2021

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

juanfont
juanfont previously approved these changes Nov 13, 2021
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
Copy link
Owner

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?

Copy link
Owner

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...

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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)
Copy link
Owner

Choose a reason for hiding this comment

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

Why 88?

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

juanfont
juanfont previously approved these changes Nov 14, 2021
Copy link
Owner

@juanfont juanfont left a comment

Choose a reason for hiding this comment

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

All good!

@kradalby
Copy link
Collaborator Author

kradalby commented Nov 15, 2021

This is ready for now. This should give us a lot of helpers to keep consistent good practice.

@kradalby
Copy link
Collaborator Author

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:
Copy link
Owner

Choose a reason for hiding this comment

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

:( this will hurt

Copy link
Collaborator Author

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

Copy link
Owner

@juanfont juanfont left a comment

Choose a reason for hiding this comment

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

Wow. Just, wow.

@kradalby kradalby merged commit 4b525a3 into juanfont:main Nov 15, 2021
@kradalby kradalby deleted the golanglint branch March 2, 2022 08:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants