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

✨ Import grouping linter changes #748

Merged
merged 2 commits into from
Apr 9, 2024

Conversation

trgeiger
Copy link
Contributor

@trgeiger trgeiger commented Apr 8, 2024

Description

This PR enables the gci linter in golangci-lint and adds configuration to group imports as follows:

  • standard library
  • 3rd party packages
  • packages from operator-framework org
  • local imports from operator-framework/operator-controller

Closes #510

Reviewer Checklist

  • API Go Documentation
  • Tests: Unit Tests (and E2E Tests, if appropriate)
  • Comprehensive Commit Messages
  • Links to related GitHub Issue(s)

golangci-lint will now group imports in the following order:
- standard library
- 3rd party packages outside of our Github org
- packages from our Github org
- local imports from operator-controller

Signed-off-by: Tayler Geiger <tayler@redhat.com>
All imports should now be properly ordered according to the gci group
import rules.

Signed-off-by: Tayler Geiger <tayler@redhat.com>
@trgeiger trgeiger requested a review from a team as a code owner April 8, 2024 21:34
Copy link

netlify bot commented Apr 8, 2024

Deploy Preview for olmv1 ready!

Name Link
🔨 Latest commit a516d89
🔍 Latest deploy log https://app.netlify.com/sites/olmv1/deploys/661462e1f189d50008e182b7
😎 Deploy Preview https://deploy-preview-748--olmv1.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

codecov bot commented Apr 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.81%. Comparing base (c8a6e3d) to head (a516d89).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #748   +/-   ##
=======================================
  Coverage   63.81%   63.81%           
=======================================
  Files          22       22           
  Lines        1415     1415           
=======================================
  Hits          903      903           
  Misses        459      459           
  Partials       53       53           
Flag Coverage Δ
e2e 47.20% <ø> (ø)
unit 57.78% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@m1kola m1kola left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks for taking care of this.

Would you be able to do similar change in other OLMv1 repos (e.g. catalogd and rukpak)?

@m1kola m1kola added this pull request to the merge queue Apr 9, 2024
Merged via the queue into operator-framework:main with commit b621279 Apr 9, 2024
17 checks passed
- standard
- default
- prefix(github.com/operator-framework)
- prefix(github.com/operator-framework/operator-controller)
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about using localmodule for implicitly handling the imports from the same module?

Suggested change
- prefix(github.com/operator-framework/operator-controller)
- localmodule

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good idea, I'll throw up another small PR soon.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would we want dot or alias groups?

Copy link
Member

Choose a reason for hiding this comment

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

I would say we do not need a separate group for those.

I would even consider disallowing dot imports.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like the localmodule section in gci hasn't hit golangci-lint yet

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.

Linting for import groups
3 participants