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

x/tools/cmd/goimports: imports current package when moving a package #12653

Closed
mstoykov opened this issue Sep 17, 2015 · 5 comments
Closed

x/tools/cmd/goimports: imports current package when moving a package #12653

mstoykov opened this issue Sep 17, 2015 · 5 comments
Labels
FrozenDueToAge Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@mstoykov
Copy link

A common case (for me at least) is for code to be written in a given package and than to be moved to another package.

Coping the code (+ changing the package) works great, but if the code was depending on the package it was moved into, go will die with import cycle.

Removing the package and running goimports adds it again - this for me happens on saving the file.

It seems reasonable that importing the current package is not desirable and in my setup (vim + vim-go) the file looks OK in vim even though it's not.

I propose goimports to remove(not add) the current package.

@ianlancetaylor ianlancetaylor changed the title goimports imports current package x/tools/cmd/goimports: imports current package when moving a package Sep 17, 2015
@ianlancetaylor
Copy link
Contributor

I'm sorry, I don't understand what you mean. Could you give a small example where goimports does the wrong thing?

@ianlancetaylor ianlancetaylor added this to the Unreleased milestone Sep 17, 2015
@mstoykov
Copy link
Author

I'm sorry too, Let's see if my examples are better than my explanations.
Somewhere in the example.com/project/utils package there is a file like:

package utils

func A() {
}

in another package we have:

package not_utils

import "example.com/project/utils"

func B() {
    utils.A() // caling the utils package
}

Wanting to move B in the utils package I go through the following steps:
moving the second file in utils, changing the package and removing the import:

package utils

func B() {
    utils.A() 
}

and then running goimports against the file(saving in vim) will put the import in again.

package utils

import "example.com/project/utils"

func B() {
    utils.A() 
}

Given that importing the package a file is from is an import cycle I think that goimports should not do it and even remove such an import.

@dmitshur
Copy link
Contributor

A few thoughts:

  • Currently, goimports only modifies your imports, and never any of your code. This is something people are used to and it has good properties like being sure your code is never modified (outside of applying gofmt formatting).

    • Minor exception, it also supposedly adds package main if you have func main() in your code and package clause is missing. But this is quite limited in scope and I've actually never seen this work successfully for me, I just heard it's a feature.
  • Importing the current package is sometimes desirable. Consider working on an external test in util_test.go:

    package util_test
    
    import "testing"
    
    import "example.com/project/utils"
    
    func TestA(t *testing.T) {
        utils.A()
    }

    In theory, goimports could know if it's dealing with an external test or not and change its behavior. But need to think if that's an improvement or not.

  • Is there a reason you choose to run goimports before removing utils. from utils.A(), since the package prefix is no longer needed after you've moved that func into the current package?

@mstoykov
Copy link
Author

goimports is automatically ran on every save of vim - a vim-go feature which has only this downside.

The problem mostly arises because I move a whole file (with not only one reference of the 'utils' package) and then I open it up in vim to change the package name - upon writing I would prefer if the current package is not imported - this will make all references to 'utils' inside the file be erroneous

I think that is better, than the current situation where it will be imported and than vim will show the file to be fine, but there will be an import cycle.

My problem is mostly with vim-go and gorename(or another tool) not supporting moving between packages.

But I still think goimports shouldn't import the current package.

I also consider 'utils_test' to be different (and special) package altogether, I don't know how goimports handles that case though.

@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Sep 12, 2019
@heschi
Copy link
Contributor

heschi commented Nov 7, 2019

I think this was resolved in #30663. Please comment if you disagree.

@heschi heschi closed this as completed Nov 7, 2019
@golang golang locked and limited conversation to collaborators Nov 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

No branches or pull requests

5 participants