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

importas: detect duplicate alias or package in the configuration #3753

Merged
merged 2 commits into from
Apr 4, 2023

Conversation

ldez
Copy link
Member

@ldez ldez commented Apr 2, 2023

Produce an error log when a package has multiple aliases:

  importas:
    no-unaliased: true
    alias:
      - alias: netv1
        pkg: "k8s.io/api/networking/v1"
      - alias: networkingv1
        pkg: "k8s.io/api/networking/v1"

Produce an error log when an alias is used for several packages:

  importas:
    no-unaliased: true
    alias:
    - alias: netv1
      pkg: "k8s.io/api/networking/v1"
    - alias: netv1
      pkg: "k8s.io/api/networking/v1beta1"

@ldez ldez marked this pull request as draft April 2, 2023 09:34
@ldez ldez added linter: update Update the linter implementation inside golangci-lint enhancement New feature or improvement labels Apr 2, 2023
@ldez ldez changed the title importas: detect multiple aliases for the same package importas: detect duplicate alias or package Apr 2, 2023
@ldez ldez changed the title importas: detect duplicate alias or package importas: detect duplicate alias or package in the configuration Apr 2, 2023
@ldez ldez marked this pull request as ready for review April 2, 2023 09:39
@ldez ldez assigned bombsimon and unassigned bombsimon Apr 2, 2023
@ldez ldez requested a review from bombsimon April 2, 2023 13:41
Copy link
Member

@bombsimon bombsimon left a comment

Choose a reason for hiding this comment

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

Should unique aliases be an opt in? F.ex. by default the linter supports unaliased imports even if it's added with alias so supporting multiple aliases feels similar (unaliased + alias1 vs. alias1 + alias2)?

Also I think since the linter uses a map this should already report a diagnostic since first adding alias1 and then alias2 will have alias2 override alias1 making imports with alias1 not allowed.

Currently this change makes the linter behave differently depending on if it's run natively or through golangci-lint which I think is nice to avoid. But maybe I'm missing some context here?

@ldez
Copy link
Member Author

ldez commented Apr 3, 2023

Currently this change makes the linter behave differently depending on if it's run natively or through golangci-lint which I think is nice to avoid. But maybe I'm missing some context here?

It's a not different behavior, it's just a better way to detect problems inside the configuration.
For me, it's not related to the behavior of the linter.
The configuration is sent to the linter without modifications related to the detection of duplication, it's just a log.

The provider configuration uses a map just because this is "easier" with CLI flag parsing.

Should unique aliases be an opt in? F.ex. by default the linter supports unaliased imports even if it's added with alias so supporting multiple aliases feels similar (unaliased + alias1 vs. alias1 + alias2)?

As the key of the map is the package, the linter is not able to handle multiple aliases because it will not see them.

The fact to use the same alias for multiple packages is the opposite of the target of this linter: use the same import aliases everywhere for one package.
The package has been mainly made to handle k8s imports: the goal is to avoid ambiguities on imports between files and inside a file.
K8s imports always end with v1, v1alpha1, v1beta1, versioned, fake, externalversions, etc. so when you are using multiple resources you are quickly forced to use explicit aliases and use the same between files.

The no-unaliased option is just because in some cases when you are using CRD inside a package, which mainly targets this CRD, you can want to use it without aliases, it's close to what the . import allows.

@bombsimon
Copy link
Member

The configuration is sent to the linter without modifications related to the detection of duplication, it's just a log.

My bad, I thought error severity would affect running or exit code but if it's just logging it makes sense.

Regarding the map, my thought was to change this behaviour if you wanted to support different behaviour and/or have this logic in the linter instead.

@ldez ldez merged commit 51f8a61 into golangci:master Apr 4, 2023
@ldez ldez deleted the feat/importas-duplicate-aliases branch April 4, 2023 07:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or improvement linter: update Update the linter implementation inside golangci-lint
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants