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

Enable gci linter with automatic fix. Apply lint fixes. #236

Merged
merged 2 commits into from
Jan 10, 2024

Conversation

guineveresaenger
Copy link
Contributor

If we want to standardize import grouping and sorting, we should enforce it in code.

@guineveresaenger guineveresaenger requested a review from a team January 8, 2024 23:05
Copy link
Member

@iwahbe iwahbe left a comment

Choose a reason for hiding this comment

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

I 100% agree that gci should flag these.

The style we normally use is:

import (
    std

    default

    github.com/pulumi/upgrade-provider/*  // (local) imports
)

I believe we can configure gci to support this.

Makefile Outdated
@@ -11,7 +11,7 @@ install:
go install github.com/pulumi/upgrade-provider

lint:
golangci-lint run
golangci-lint run -E gci --fix
Copy link
Member

Choose a reason for hiding this comment

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

My prior is that make lint shouldn't make code changes. Instead changes should be delegated to another command, such as make lint.fix.

-E -> --enable because I didn't know what -E meant, but --enable is readable.

Suggested change
golangci-lint run -E gci --fix
golangci-lint run --enable gci

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that's totally fair - I think a good state is where we can run make lint and see necessary changes.

@guineveresaenger
Copy link
Contributor Author

Do we have these standards documented somewhere? I looked into it and we can definitely configure custom rules.

@iwahbe
Copy link
Member

iwahbe commented Jan 9, 2024

Do we have these standards documented somewhere? I looked into it and we can definitely configure custom rules.

Not that I'm aware of. I see two patterns in our code bases:

  1. The one that I mentioned above. (example)
  2. A variation, where there is an additional block for github.com/pulumi/*. (example)

I'm fine with either.

This change adds linting for Go import sections following current
practices in our repositories of grouping by the following custom
order:

- standard library
- blank imports
- default imports
- imports for Pulumi libraries
- local package imports
Copy link
Member

@iwahbe iwahbe left a comment

Choose a reason for hiding this comment

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

Awesome! LGTM!

@guineveresaenger guineveresaenger merged commit 78a682a into main Jan 10, 2024
2 checks passed
@guineveresaenger guineveresaenger deleted the guin/enable-gci-lint branch January 10, 2024 00:24
guineveresaenger added a commit to pulumi/pulumi-aiven that referenced this pull request Jan 14, 2024
This PR is part of a migration to standardize the order and grouping of
our Go imports. See pulumi/upgrade-provider#236.
guineveresaenger added a commit to pulumi/pulumi-akamai that referenced this pull request Jan 16, 2024
This PR is part of a migration to standardize the order and grouping of
our Go imports. See pulumi/upgrade-provider#236.
guineveresaenger added a commit to pulumi/pulumi-alicloud that referenced this pull request Jan 16, 2024
This PR is part of a migration to standardize the order and grouping of
our Go imports. See pulumi/upgrade-provider#236.
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