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

gci: Add trailing slash to local module #4608

Closed
5 tasks done
StephenBrown2 opened this issue Apr 2, 2024 · 10 comments
Closed
5 tasks done

gci: Add trailing slash to local module #4608

StephenBrown2 opened this issue Apr 2, 2024 · 10 comments
Labels
dependencies Relates to an upstream dependency

Comments

@StephenBrown2
Copy link
Contributor

StephenBrown2 commented Apr 2, 2024

Welcome

Description of the problem

In my company's package, there are a few repositories that are named similarly, because they are related, and one is imported in the other (for example, a separate proto repository).

e.g.:
Main program: github.com/Company/main-program
Dependency: github.com/Company/main-program-dependency

This unfortunately confuses gci when trying to use the localmodule section, as imports from both repos get grouped as one.

e.g:

import (
  "fmt"
  "errors"

  "github.com/Company/main-program/package"
  "github.com/Company/main-program-dependency/other"
)

When they should be separate, e.g:

import (
  "fmt"
  "errors"

  "github.com/Company/main-program-dependency/other"

  "github.com/Company/main-program/package"
)

Adding the local module as it's own custom section with the trailing slash gets around this (i.e. prefix("github.com/Company/main-program/")), but I would prefer to rely in the linter's detection so I can use the same config across multiple repos in our org.

This could be done by modifying https://github.com/golangci/golangci-lint/blob/master/pkg/golinters/gci.go#L169 to be:

v.Path = info.Path + "/"

Version of golangci-lint

$ golangci-lint --version
golangci-lint has version v1.57.3-0.20240402181201-2dd62d125014 built with go1.21.8 from (unknown, modified: ?, mod sum: "h1:jmz5tBAul6h97PilGm8Sd+n9wWfIWduO67xL9AAOF0k=") on (unknown)

Configuration

---
linters:
  enable:
    - gci
linters-settings:
  gci:
    sections:
      - standard
      - default
      - blank
      - dot
      - "prefix(github.com/Company/)"
      - localmodule # Package name being "github.com/Company/main-program"

Go environment

$ go version && go env
go version go1.21.8 linux/amd64
GO111MODULE=''
GOARCH='amd64'
GOBIN=''
GOCACHE='/home/user/.cache/go-build'
GOENV='/home/user/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/home/user/go/pkg/mod'
GONOPROXY='github.com/Company'
GONOSUMDB='github.com/Company'
GOOS='linux'
GOPATH='/home/user/go'
GOPRIVATE='github.com/Company'
GOPROXY='direct'
GOROOT='/usr/lib/golang'
GOSUMDB='off'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/usr/lib/golang/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.21.8'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='1'
GOMOD='/dev/null'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build2947470614=/tmp/go-build -gno-record-gcc-switches'

Verbose output of running

$ golangci-lint cache clean
$ golangci-lint run -v
# paste output here

A minimal reproducible example or link to a public repository

An example, non-functioning, code sample.

package main

import (
	"fmt"

	"github.com/Company/main-program-dependency/other"
	"github.com/Company/main-program/pkg"
)

func main() {
	fmt.Println("Hello, World!")
	pkg.Package(other.Name)
}

Expected:

package main

import (
	"fmt"

	"github.com/Company/main-program-dependency/other"

	"github.com/Company/main-program/pkg"
)

func main() {
	fmt.Println("Hello, World!")
	pkg.Package(other.Name)
}

Validation

  • Yes, I've included all information above (version, config, etc.).
@StephenBrown2 StephenBrown2 added the bug Something isn't working label Apr 2, 2024
@ldez ldez added dependencies Relates to an upstream dependency and removed bug Something isn't working labels Apr 2, 2024
@ldez
Copy link
Member

ldez commented Apr 2, 2024

Hello,

I recommend opening an issue to the linter repo: https://github.com/daixiang0/gci

golangci-lint is a wrapper around linter.

@ldez ldez closed this as completed Apr 2, 2024
@StephenBrown2
Copy link
Contributor Author

StephenBrown2 commented Apr 2, 2024

Agreed that golangci-lint is a wrapper around linters. However, your hack #4522 (comment) is what I'm proposing to be modified: https://github.com/golangci/golangci-lint/pull/4522/files#diff-298e405f64d9b95decc7390b518fc1bd580eb8aee31a7eb7d7e83f5ca6527e6aR154-R169

@ldez
Copy link
Member

ldez commented Apr 2, 2024

In some way, my hack is just what the linter does, the problem is not my hack but how localModule is handled inside the linter.

@StephenBrown2 StephenBrown2 changed the title gci: Add trailing slash to local module gci: Add trailing slash to local module (golangci-lint hack) Apr 2, 2024
@ldez ldez changed the title gci: Add trailing slash to local module (golangci-lint hack) gci: Add trailing slash to local module Apr 2, 2024
@StephenBrown2
Copy link
Contributor Author

In some way, my hack is just what the linter does

Then the hack should be removed, no?

@ldez
Copy link
Member

ldez commented Apr 2, 2024

Then the hack should be removed, no?

No, because the linter doesn't work inside golangci-lint without this hack.

The problem is here: https://github.com/daixiang0/gci/blob/4f7ff3c79e25e9076af03eab88e658d7448ae7de/pkg/section/local_module.go#L21

@ldez
Copy link
Member

ldez commented Apr 2, 2024

The fix:

if spec.Path == m.Path || strings.HasPrefix(spec.Path, m.Path+"/") {

instead of:

if strings.HasPrefix(spec.Path, m.Path) {

@StephenBrown2
Copy link
Contributor Author

Thanks for that, I was about to make a different PR: daixiang0/gci#203

@ldez
Copy link
Member

ldez commented Apr 2, 2024

I think your PR will create a side effect with a module without packages.
Because the problem is the matching, not the value of path.

My PR fixes the problem at the root without any side effects.

@StephenBrown2
Copy link
Contributor Author

Thanks. Your understanding is better than mine. I hope it can be merged to gci soon.

@ldez
Copy link
Member

ldez commented Apr 2, 2024

I added tests into the PR to illustrate the problem with your PR: daixiang0/gci@005b33c

You can close your PR because it will create a regression.

Note: localModule has the same problem as prefix with a submodule that uses the root module name (submodule name: example.com/hello/world, root module name: example.com/hello).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Relates to an upstream dependency
Projects
None yet
Development

No branches or pull requests

2 participants