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

[Linting] Enforce 120 max lines, and import order with custom linting #319

Closed
wants to merge 65 commits into from

Conversation

h5law
Copy link
Contributor

@h5law h5law commented Jan 12, 2024

Summary

Human Summary

  • Enforce max lines as 120 in all files except errors.go files (using lll)
  • Enforce import order (std, external, internal) (using gci)
    • Excludes generated files
  • Enforce gofumpt formatting
  • Disable enforcement for certain files
  • Create workflow for this for CI
  • Fixes all linter errors for the entire repo so we can enable them for new PRs only

make go_gci will order all eligible .go files imports to be std,external,internal
- here non eligible files are ones with ignite scaffold comments you need to do them yourself as gci will remove the comment
make go_gofumpt will format (scricter than gofmt) all eligible '.go' files
- non-eligible files mostly include generated files like mocks and protobuf generated code
make go_lint will show any linter errors

These three makefile targets rely on you first running make install_ci_deps which will install golangci-lint, gci and gofumpt if you don't already have them. These are configured in the .golangci.yml file and the workflow file for running tests.

From now on PRs will (🤞🏼) show checks not comments where you have linter errors.

Issue

  • Fixes N/A

Type of change

Select one or more:

  • New feature, functionality or library
  • Bug fix
  • Code health or cleanup
  • Documentation
  • Other (specify)

Testing

  • Run all unit tests: make go_develop_and_test
  • Run E2E tests locally: make test_e2e
  • Run E2E tests on DevNet: Add the devnet-test-e2e label to the PR. This is VERY expensive, only do it after all the reviews are complete.

Sanity Checklist

  • I have tested my changes using the available tooling
  • I have performed a self-review of my own code
  • I have commented my code, updated documentation and left TODOs throughout the codebase

@h5law h5law added the code health Cleans up some code label Jan 12, 2024
@h5law h5law added this to the Shannon TestNet milestone Jan 12, 2024
@h5law h5law self-assigned this Jan 12, 2024
Makefile Outdated Show resolved Hide resolved
e2e/tests/init_test.go Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
Copy link
Member

@Olshansk Olshansk left a comment

Choose a reason for hiding this comment

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

Halfway through

tools/scripts/gofumpt/main.go Show resolved Hide resolved
tools/scripts/gofumpt/main.go Show resolved Hide resolved
tools/scripts/gci/filters/filters.go Outdated Show resolved Hide resolved
tools/scripts/gci/filters/filters.go Show resolved Hide resolved
@h5law h5law force-pushed the feat/linting-updates branch from 552e943 to 0ddd1c4 Compare January 20, 2024 04:11
Copy link
Contributor

@red-0ne red-0ne left a comment

Choose a reason for hiding this comment

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

Thanks for going through all the codebase 🙏

@@ -2,7 +2,6 @@ package keeper

import (
"context"
"fmt"
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to be required

@Olshansk
Copy link
Member

@h5law Should we move this one to a draft state if it's not under active review/development?

@h5law
Copy link
Contributor Author

h5law commented Mar 12, 2024

This is probably a good community task as picking up from what I started shouldn't be too hard with the tooling added. Moved into draft and put into iteration 14 «Developer Experience»

@h5law h5law added the community A ticket intended to potentially be picked up by a community member label Mar 12, 2024
@Olshansk
Copy link
Member

This is probably a good community task as picking up from what I started shouldn't be too hard with the tooling added.
Moved into draft and put into iteration 14 «Developer Experience»

@h5law I'd rather have someone on the team do this. When you introduce a (trivial) change that touches a large surface area of the codebase, it's better to be done by the core team.

Pick it up in iteration 14 and we should be 🏅

@h5law h5law removed the community A ticket intended to potentially be picked up by a community member label Mar 13, 2024
@Olshansk
Copy link
Member

Olshansk commented Apr 5, 2024

Outdated.

@Olshansk Olshansk closed this Apr 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code health Cleans up some code
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

5 participants