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

Enforce import order #254

Merged
merged 4 commits into from
Jul 31, 2023
Merged

Enforce import order #254

merged 4 commits into from
Jul 31, 2023

Conversation

barrettj12
Copy link
Contributor

Apparently there is an implicit rule that Pebble imports should be arranged into three groups:

  • standard library imports
  • 3rd-party / non-Pebble imports
  • Pebble imports (i.e. those beginning with github.com/canonical/pebble)

This rule was not being enforced anywhere except code review, and there were many Go files not following this convention.

I've fixed up all Go files so that they now follow this convention. I used the gci tool to enforce the import order. Unfortunately goimports is not strict enough - it will only make sure that imports from the same group are not together. It will allow a group to be split up, so e.g. the following still passes:

import (
  "github.com/canonical/pebble/internals/plan"

  "github.com/canonical/pebble/internals/servicelog"
)

The easiest way to use gci is via golangci-lint (which gci is bundled inside). Run

golangci-lint run -c .github/.golangci.yml

to do the linting. If you add the --fix flag, gci will fix all the imports for you.

While I'm here, I resolved a stale TODO in daemon_test.go by changing the import of gocheck to use ., following the convention observed in (most) other test files.

Finally, I've added a Lint GitHub workflow to enforce this import order on future PRs.

All Go files should have exactly three import groups:
- stdlib
- 3rd-party / non-project imports
- project imports (from inside canonical/pebble)
gocheck should be imported with `.`, as suggested in the comment.
Copy link
Contributor

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

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

Thanks! Definitely nice to enforce this via tooling rather than (clearly error-prone) human eyeballs. And nice to make daemon_test.go consistent and get rid of that TODO too.

.github/workflows/lint.yml Outdated Show resolved Hide resolved
.github/workflows/lint.yml Show resolved Hide resolved
@flotter
Copy link
Contributor

flotter commented Jul 5, 2023

Just an idea for the future: I like the idea of having a single script (like snapd and others : run-checks) that can perform the same checks the CI will in advance to save time iterating. Right now things are still simple, but I think it would soon be really useful to start adding spread like tests as well, and this means the turnaround time of a failed CI run can be painfully long.

HACKING.md Show resolved Hide resolved
Add linting to the pre-merge checks to enforce Go import groups:
- stdlib
- 3rd-party / non-Pebble imports
- Pebble imports
Copy link
Member

@jnsgruk jnsgruk left a comment

Choose a reason for hiding this comment

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

Looks good, thanks Jordan. Merging this as it's just an enforcement of agreed code style.

@jnsgruk jnsgruk merged commit 731299e into canonical:master Jul 31, 2023
@barrettj12 barrettj12 deleted the gci branch August 1, 2023 00:42
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.

4 participants