Handle newlines in blocks caused by conflict with goimports

[avorima]

Multi-line comments separated with newlines cause conflicts with goimports.

Given:

package proc

import (
    "github.com/a/b"
    "github.com/local/foo"

    // A multi-line comment explaining what this dependency
    // is used for or what it initializes.
    _ "github.com/client/auth"
)

sections:
  - Standard
  - Default
  - Prefix(github.com/local)

The result is:

package proc

import (
    "github.com/a/b"
    // A multi-line comment explaining what this dependency
    // is used for or what it initializes.
    _ "github.com/client/auth"

    "github.com/local/foo"
)

But goimports will format it as:

package proc

import (
    "github.com/a/b"

    // A multi-line comment explaining what this dependency
    // is used for or what it initializes.
    _ "github.com/client/auth"

    "github.com/local/foo"
)

This means that the two cannot be used together, e.g. with golangci-lint.

[daixiang0]

If you want gci work with goimports, you should add Prefix(github.com/a) and Prefix(github.com/client), since they are belonged to the same type but a different way to group.

[avorima]

This only affects 1 file though. Right now I have a few workarounds for this, but having to force this for every file in the project to fix it for 1 is not practical I think.

[daixiang0]

Although there is a conflict, golangci-lint would return a definite result. Also, you can skip use GCI for some files with golangci-lint config.

[avorima]

Yes, that is one workaround. Still, the actual gci invocation has to be adapted to ignore that one file. It would be great if golangci-lint could apply the fixes from gci like with goimports for example

[daixiang0]

Actually goimports would add an empty line before comments(not only for multi-lines) which break the sections from gci:

"fmt"
// test1
"go/test1"
// test2
"go/test2"

to

"fmt"

// test1
"go/test1"
// test2
"go/test2"

I really do not like this behavior of goimports.

[daixiang0]

A workaround is that put comments after import.

I do not know why goimports does it.

[daixiang0]

Close now since no plan to fix, unless more users require it.

[howardjohn]

This currently is still pretty broken for us. While we can get it working by moving the comment inline ("foo.bar" // my comment), this is not auto-fixed so we end up with much developer pain.

[daixiang0]

@howardjohn I think you can try without goimports, since its import completion is useless in most cases(IDE will do it, checking it in CI is unnecessary, and any import missing will break UT).

[howardjohn]

goimports is a requirement for our use cases

[daixiang0]

@howardjohn I am working on compatible with goimports, also I want to know how you use GCI, single or within golangci-lint？

[howardjohn]

We use in golangci-lint primarily but I personally use it directly as well (faster)

On Mon, Aug 29, 2022, 7:55 PM Loong Dai @.***> wrote:

@howardjohn https://github.com/howardjohn I am working on compatible with goimports, also I want to know how you use GCI, single or within golangci-lint？

— Reply to this email directly, view it on GitHub https://github.com/daixiang0/gci/issues/76#issuecomment-1231081835, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEYGXI7T5OG436HYB7OGTTV3VZ3ZANCNFSM53SB6AEQ . You are receiving this because you were mentioned.Message ID: @.***>

[daixiang0]

@howardjohn @avorima could you give the smallest GO file to reproduce it?

[avorima]

My initial issue comment would be such an example, but I think the main problem for me was the general handling of comments of gci. These problems have since been fixed and I can live with having to move comments inline. So I don't really have any stakes in this anymore.

[howardjohn]

package main

import (
    _ "fmt"
    _ "io"
    _ "net/http"
    _ "os"
    _ "path/filepath"

    _ "k8s.io/apimachinery/pkg/runtime/serializer"
    _ "k8s.io/client-go/kubernetes"
    //  allow out of cluster authentication
    _ "k8s.io/client-go/plugin/pkg/client/auth"
    _ "k8s.io/client-go/rest"
    _ "k8s.io/client-go/tools/clientcmd"

    _ "istio.io/pkg/version"
)

golangci-lint run --fix -c ./common/config/.golangci-format.yml ./main.go

# WARNING: DO NOT EDIT, THIS FILE IS PROBABLY A COPY
#
# The original version of this file is located in the https://github.com/istio/common-files repo.
# If you're looking at this file in a different repo and want to make a change, please go to the
# common-files repo, make the change there and check it in. Then come back to this repo and run
# "make update-common".

service:
  # When updating this, also update the version stored in docker/build-tools/Dockerfile in the istio/tools repo.
  golangci-lint-version: 1.38.x # use the fixed version to not introduce new linters unexpectedly
run:
  # timeout for analysis, e.g. 30s, 5m, default is 1m
  deadline: 20m
  build-tags:
  - integ
  - integfuzz
  # which dirs to skip: they won't be analyzed;
  # can use regexp here: generated.*, regexp is applied on full path;
  # default value is empty list, but next dirs are always skipped independently
  # from this option's value:
  #     vendor$, third_party$, testdata$, examples$, Godeps$, builtin$
  skip-dirs:
  - genfiles$
  - vendor$

  # which files to skip: they will be analyzed, but issues from them
  # won't be reported. Default value is empty list, but there is
  # no need to include all autogenerated files, we confidently recognize
  # autogenerated files. If it's not please let us know.
  skip-files:
  - ".*\\.pb\\.go"
  - ".*\\.gen\\.go"

linters:
  disable-all: true
  enable:
  - goimports
  - gofumpt
  - gci
  fast: false

linters-settings:
  gci:
    sections:
      - standard # Captures all standard packages if they do not match another section.
      - default # Contains all imports that could not be matched to another section type.
      - prefix(istio.io/) # Groups all imports with the specified Prefix.
  goimports:
    # put imports beginning with prefix after 3rd-party packages;
    # it's a comma-separated list of prefixes
    local-prefixes: istio.io/

issues:

  # Maximum issues count per one linter. Set to 0 to disable. Default is 50.
  max-per-linter: 0

  # Maximum count of issues with the same text. Set to 0 to disable. Default is 3.
  max-same-issues: 0

Will constantly add and remove the extra line

[howardjohn]

Or remove --fix and it will fail on gci or goimports depending on space exist or not exist

[mholt]

We just bumped into this in https://github.com/caddyserver/caddy/pull/6252 FYI

[daixiang0]

@mholt thanks for input, I recommand run goimports then gci as README said.

[mholt]

But then it will fail goimports.