segmentio / golines

A golang formatter that fixes long lines
MIT License
903 stars 56 forks source link

Unnecessary addition of import logger when symbol exists within a package. #115

Open TafThorne opened 9 months ago

TafThorne commented 9 months ago

As part of a large module (1000+files, 50k+ source lines, 50k+ test lines) I recently tried to use golines but found it frequently adding new sources for a logger symbol. Within the project we import a logging package from a third party module, declare an instance of it in some of our packages and then refer to it from multiple files within the package. Golines seems to not recognise the presence of this private variable from the module and tries to import extra logger instances instead.

I have not seen this happen with any other package. It does not happen with all instances of logger in all packages, only some of them within the module. I cannot spot a consistent pattern in which do or do not show the problem. I have not been able to reproduce it outside of the wider project & I cannot share the source code seeing the issue. I realise that this does not give you much to go on.

I see the project in the large project when doing something like the following:

A file in package instantiating a logger.

package example

import "example.com/logging"

var logger = logging.LoggerType{}

Using the logger in another file within the same package

package example

func Example() {
    logger.Log("An example message")
}

Golines will want import an extra logger symbol in the second file.


As a work around I have found that renaming logger to log stops the issue. var log = logging.LoggerType{} works fine.

telemachus commented 9 months ago

My guess is that this isn't golines per se but goimports (which is the default formatter for golines.)

Does the problem go away if you run golines with the argument --base-formatter=gofmt (or --base-formatter=gofumpt)?

TafThorne commented 9 months ago

golines -m 120 --base-formatter=gofmt -w . did not show the problem.

When I run goimports -w . directly I do not see the problem.

Running golines -m 120 --base-formatter=goimports -w . recreates the issue.

telemachus commented 9 months ago

That's pretty much what I thought. As I said, the issue is not coming from golines—at least not primarily. I think the easiest solution for you is to explicitly exclude goimports from golines by using the --base-formatter setting.

(As for why goimports directly doesn't show the same problem, I'm not sure, but here's my best guess. golines may be picking up a different versions of golines than your shell because the environment differs in different situations (CI? test servers? etc.). You can see the code where golines picks a base formatter here.)