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

Fix removal of kubebuilder imports marker #135

Open
avorima opened this issue Feb 2, 2023 · 14 comments
Open

Fix removal of kubebuilder imports marker #135

avorima opened this issue Feb 2, 2023 · 14 comments
Labels
bug Something isn't working help wanted Extra attention is needed

Comments

@avorima
Copy link
Contributor

avorima commented Feb 2, 2023

What version of GCI are you using?

v0.9.0

Reproduce Steps

package main

import (
	"os"
	"fmt"
	// +kubebuilder:scaffold:imports
)

Run gci print .

What did you expect to see?

package main

import (
        "fmt"
        "os"
        // +kubebuilder:scaffold:imports
)

What did you see instead?

package main

import (
        "fmt"
        "os"
)

This breaking change was introduced in #120. You can verify by checking the output of v0.8.2 vs v0.8.3.

@avorima
Copy link
Contributor Author

avorima commented Feb 2, 2023

I think this is an issue with comments in general. Consider the following:

package main

import (
        "os" // test
        // comment
        "fmt"
)

This turns into:

package main

import ( // comment
        "fmt"
        "os" // test
)

Instead of:

package main

import (
        // comment
        "fmt"
        "os" // test
)

@daixiang0 daixiang0 added bug Something isn't working help wanted Extra attention is needed labels Feb 6, 2023
@daixiang0
Copy link
Owner

Is the marker always isolated?

@piepmatz
Copy link

piepmatz commented Feb 6, 2023

Is the marker always isolated?

In the sense of "on a separate line"? Then yes.

@reggieriser
Copy link

To add one more data point, once your imports have been reformatted to be in the odd state shown above:

import ( // comment
        "fmt"
        "os" // test
)

and you run gci again, then the top comment gets removed entirely:

import (
	"fmt"
	"os" // test
)

I'm using gci through golangci-lint. This started happening once I upgraded golangci-lint to v1.51.0+.

@daixiang0
Copy link
Owner

Is the marker always isolated?

In the sense of "on a separate line"? Then yes.

As I said in the README, isolated comment is hard to handle, for now I choose delete them.

As kubebuilder marker is useful, I consider the hardcode way to keep it.

@daixiang0
Copy link
Owner

@avorima I do not see this marker in https://book.kubebuilder.io/reference/markers.html.

@daixiang0
Copy link
Owner

wait for kubernetes-sigs/kubebuilder#1487 to get more info to decide how to deal with those markers.

@daixiang0
Copy link
Owner

#136 would fix the general issue.

@avorima
Copy link
Contributor Author

avorima commented Feb 7, 2023

goimports is able to handle isolated comments

@daixiang0
Copy link
Owner

goimports is able to handle isolated comments

I think goimports just leave them as they were.

@avorima
Copy link
Contributor Author

avorima commented Feb 13, 2023

Yes, it looks that way

@cnmcavoy
Copy link

cnmcavoy commented Apr 4, 2023

I also see gci removing go generate lines in specific scenarios. Consider:

package main
 
 import (
        "foo.com/example/service"
 )
// generated config (go generate ./... to regenerate)
//go:generate ./config.sh
import _ "foo.com/example/config"

Becomes:

package main

 import (
        "foo.com/example/service"
         _ "foo.com/example/config"
 )

Which is very different behavior due to the lost go:generate directives.

@t0rr3sp3dr0
Copy link

Is there some workaround for this problem?

@chadr123
Copy link

Is there any update? I'm still using old version of gci not latest version due to this issue. :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

7 participants