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/refactor/rename: renaming struct should rename method receivers that follow naming advice #18728

Open
myitcv opened this issue Jan 20, 2017 · 7 comments
Labels
Refactoring Issues related to refactoring tools Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@myitcv
Copy link
Member

myitcv commented Jan 20, 2017

Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (go version)?

go version go1.7.4 linux/amd64

What operating system and processor architecture are you using (go env)?

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/myitcv/mgo/src/modelogiq.com/g/_vendor:/home/myitcv/mgo"
GORACE=""
GOROOT="/home/myitcv/gos"
GOTOOLDIR="/home/myitcv/gos/pkg/tool/linux_amd64"
CC="gcc"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build108524765=/tmp/go-build -gno-record-gcc-switches"
CXX="g++"
CGO_ENABLED="1"

Using commit fcfba28e23c7bbd8474b355ca7d6a9d88afcca00 of golang.org/x/tools/refactor/rename

What did you do?

Starting with code:

// main.go

package main

import (
        "fmt"
)

type banana struct{}

func (b *banana) doit() {}

func main() {
        fmt.Println("Hello, playground")
}

Invoke:

gorename -offset main.go:#51 -to apple

What did you expect to see?

rename should also rename the receiver of the method doit from b -> a

The heuristic here should be that if the receiver does follow the generally accepted naming advice then rename can safely proceed and rename receivers.

What did you see instead?

Renaming banana -> apple leaves the receiver of method doit as b

@myitcv
Copy link
Member Author

myitcv commented Jan 23, 2017

@cznic just noticed you gave this a 👎

What's your thinking behind this not being the right thing to do?

@cznic
Copy link
Contributor

cznic commented Jan 23, 2017

If I command some tool to rename a particular identifier x to y I want it to do exactly and precisely that or tell me why it cannot do it. I never ever want any magic involved unless I specifically ask for magic, which I basically never do nor want. If I want to also rename the receiver then I want to explicitly ask for one more rename, and then again, I want to have full control over the new receiver name even though the magic one could be the same accidentally. But once in a while I may want a different name for the reveiver and if the magic is there unconditionally, the tool is broken and usable no more.

@cespare
Copy link
Contributor

cespare commented Jan 23, 2017

I agree that renaming a type shouldn't rename the method receivers (it does feel too "magical", to borrow @cznic's word) but it would be useful for rename (or some other tool) to have the ability to rename the receiver of all methods of a type from x to y. From reading the gorename docs, it seems like you'd currently have to do a separate rename on every method.

@myitcv
Copy link
Member Author

myitcv commented Jan 23, 2017

@cznic thanks, not unsurprising feedback now that I think about it.

I agree that just because I happen to find myself, more often than not, in the situation where I do want receivers that follow the generally accepted naming advice to also be renamed, doesn't preclude either:

  • there being situations when I don't want this behaviour
  • this not being the predominant position of everyone

On reflection, making this (the renaming of receivers) the default behaviour would almost certainly be wrong.

Presumably putting it behind a flag would work?

@cespare yes I agree that at present this is only possible via require multiple renames... which need to be done in serial and hence take a not insignificant amount of time for types with many receivers (if we ignore for one second that I'm not aware of a wrapping program that coordinates the renames). My thinking in pushing this back to rename was therefore that we benefit from a single run of types analysis.

But maybe my assumption that this would regularly be of use to the wider community is invalid?

@Dr-Terrible
Copy link

Dr-Terrible commented Jan 25, 2017

But maybe my assumption that this would regularly be of use to the wider community is invalid?

I found myself in the same boat: lot of receivers to rename that I cannot easily automated with rename, thus consuming a not insignificant amount of my time.

Presumably putting it behind a flag would work?

A flag is good enough for me, I really don't mind, provided rename offers a faster solution for these king of scenarios :)

@gopherbot gopherbot added this to the Unreleased milestone Mar 21, 2017
@anacrolix
Copy link
Contributor

I agree with @cznic: The behaviour should not be automatic. However I do desire to rename a bunch of receivers automatically. How can I do that with the tool currently? I'm trying something like:

$ gorename -from '"github.com/libp2p/go-libp2p-kad-dht".*::dht' -to node
gorename: -from "\"github.com/libp2p/go-libp2p-kad-dht\".*": invalid expression

And also:

$ gorename -from '"github.com/libp2p/go-libp2p-kad-dht"::dht' -to node
gorename: ambiguous specifier dht matches var at handlers.go:293:7, var at dht_net.go:47:7, var at routing.go:647:7, var at routing.go:456:7, var at dht.go:256:7, var at records.go:98:7, var at routing.go:570:7, var at dht.go:367:7, var at handlers.go:237:7, var at dht_bootstrap.go:113:7, var at dht.go:307:7, var at dht_bootstrap.go:73:7, var at routing.go:38:7, var at dht.go:168:7, field at query.go:24:2, var at dht.go:372:7, var at dht_test.go:171:3, var at dht.go:275:7, var at handlers.go:209:7, var at dht.go:111:2, field at dht_net.go:196:2, var at dht_test.go:767:10, var at dht.go:362:7, var at handlers.go:89:7, var at routing.go:151:7, var at dht.go:73:2, var at dht_net.go:158:7, var at dht.go:376:7, var at routing.go:113:7, var at dht_test.go:819:10, var at notif.go:45:2, var at handlers.go:47:7, var at handlers.go:341:7, var at dht_test.go:603:9, var at dht_net.go:51:7, var at dht_test.go:700:9, var at dht.go:144:7, var at query.go:42:7, var at routing.go:398:7, var at dht.go:227:7, var at dht.go:246:7, var at routing.go:273:7, var at dht_net.go:113:7, var at routing.go:467:7, var at notif.go:17:2, var at dht_net.go:149:7, var at records_test.go:46:2, var at dht_bootstrap.go:99:7, var at dht_test.go:779:3, var at routing.go:474:7, var at handlers.go:28:7, var at dht_test.go:689:9, var at lookup.go:56:7, var at dht.go:332:7, var at notif.go:77:2, var at routing.go:438:7, var at dht.go:326:7, var at records_test.go:20:2, var at dht.go:269:7, var at dht_test.go:797:22, var at dht_test.go:712:9, var at routing.go:253:7, var at dht.go:99:2, var at handlers.go:242:7, var at dht_test.go:569:10, var at dht_net.go:136:7, var at dht_bootstrap.go:48:7, var at dht.go:203:7, var at records.go:77:7, var at dht_bootstrap.go:127:7, var at handlers.go:148:7, var at dht.go:285:7, var at dht_test.go:459:2, var at records.go:26:7

@lbordowitz
Copy link

lbordowitz commented Feb 6, 2019

What about a style guide? An optional receiver style map could be written or generated which maps given types to their respective, unique receiver name. Whether during a refactor-rename, or even during the ordinary go fmt command, it could ensure that all receivers are consistently named.

@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Sep 12, 2019
@adonovan adonovan added the Refactoring Issues related to refactoring tools label Apr 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Refactoring Issues related to refactoring tools Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

No branches or pull requests

8 participants