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

Open
MStoykov opened this issue Sep 17, 2015 · 4 comments

Comments

@MStoykov
Copy link

commented Sep 17, 2015

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

This comment has been minimized.

Copy link
Contributor

commented Sep 17, 2015

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

This comment has been minimized.

Copy link
Author

commented Sep 18, 2015

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

This comment has been minimized.

Copy link
Member

commented Sep 19, 2015

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

This comment has been minimized.

Copy link
Author

commented Sep 20, 2015

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 label Sep 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.