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

.golangci: update linter rules #92

Merged
merged 2 commits into from
Nov 21, 2023
Merged

.golangci: update linter rules #92

merged 2 commits into from
Nov 21, 2023

Conversation

ellemouton
Copy link
Member

Update linter rules & fix some issues.

Copy link
Contributor

@ViktorTigerstrom ViktorTigerstrom left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this! Looks good to me, and fixes the previous issues in #88 after it was rebased 🙏🔥. Though I also would like to check if we should add some rule exceptions to counter the most recent linter issues in #88:

  1. Should we add a godox exception for this?
gbn/timeout_manager.go:87: gbn/timeout_manager.go:87: Line contains TODO/BUG/FIXME: "TODO(viktor): In the future, we may want..." (godox)
	// TODO(viktor): In the future, we may want to use this to keep track of
  1. Should we add an exception for the below as well, as changing the packet to gbn_test doesn't work?
    Error: gbn/timeout_manager_test.go:1:9: package should be `gbn_test` instead of `gbn` (testpackage)

- goerr113

# We want to allow short variable names.
- varnamelen
Copy link
Contributor

Choose a reason for hiding this comment

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

🙏

@ellemouton
Copy link
Member Author

@ViktorTigerstrom - added those 👍

Copy link
Contributor

@ViktorTigerstrom ViktorTigerstrom left a comment

Choose a reason for hiding this comment

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

tACK LGTM!

Though I'm just also mentioning a few linter ignores I've added in #87 & #88.
But IMO I do think these should actually be enforced by the linter, and it's correct that we need to ignore them when necessary though. Let me know if you think we should add rules to not enforce them though!

Linter rule to avoid adding random magic numbers
https://github.com/ViktorTigerstrom/lightning-node-connect/blob/95cb4299933ea2d49983cdf6237f140906c6e333/gbn/gbn_conn.go#L575

Linter rule for requiring an if statement rather than a single clause select statement. The reason I've kept it here is that we'll add more cases to the select statement soon:
https://github.com/ViktorTigerstrom/lightning-node-connect/blob/95cb4299933ea2d49983cdf6237f140906c6e333/gbn/timeout_manager.go#L91

Linter rule to avoid complicated functions with too many cases. In this specific case I don't agree that the function is too complicated though, and breaking it into more sub-functions rather hurts the readability rather than improving it.
https://github.com/ViktorTigerstrom/lightning-node-connect/blob/95cb4299933ea2d49983cdf6237f140906c6e333/gbn/queue.go#L301C4-L301C4

Copy link
Member

@bhandras bhandras left a comment

Choose a reason for hiding this comment

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

LGTM 🍰

@ellemouton ellemouton merged commit 9cf86bc into master Nov 21, 2023
5 checks passed
@ellemouton ellemouton deleted the updateLinterRUles branch November 21, 2023 17:10
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.

3 participants