go-mode.el icon indicating copy to clipboard operation
go-mode.el copied to clipboard

Support for gofumports

Open theckman opened this issue 5 years ago • 3 comments

Hi there,

I wanted to see whether you'd be interested in having support for gofumports. It's a goimports version of gofumpt, which means it also needs the -srcdir command line argument passed to it (much like goimports).

The issue is that if the command is anything but goimports, go-mode treats it like gofmt. I have tested that it works by symlinking goimports to gofumports, but it feels dirty.

The simplest solution seems to be to turn this to an or statement and have it also look for gofumports here: https://github.com/dominikh/go-mode.el/blob/10d6ab43d9fb308950c30b8449ec8b366c8df720/go-mode.el#L1877-L1878

I was thinking something like this, but I'm an elisp noob and haven't tested it:

(defun gofmt--is-goimports-p ()
  (let ((cmdbase (file-name-base gofmt-command)))
        (or (string-equal cmdbase "goimports")
            (string-equal cmdbase "gofumports"))))

What do you think? I can raise a PR If that seems okay.

theckman avatar Apr 18 '20 04:04 theckman

It makes sense to be able to swap in other commands that are the same "shape" as goimports.

My only suggestion is that we should make the list of commands that take a -srcdir flag a customizable variable so if you've got a really obscure fork of goimports you can just add it to that list instead of making a change to the (gofmt--is-goimports-p) function.

Something like:

(defcustom gofmt-srcdir-commands '("goimports" "gofumports")
  "Gofmt-like commands that require the -srcdir flag"
  :type '(repeat string)
  :group 'go)

(defun gofmt--is-goimports-p ()
  (let ((cmdbase (file-name-base gofmt-command)))
    (member cmdbase gofmt-srcdir-commands)))

psanford avatar Apr 18 '20 19:04 psanford

Sorry for the delay, dealing with other workstation issues. 😛

Definitely! I can take a look at doing that and raising a PR after testing that it works as expected.

theckman avatar Apr 21 '20 23:04 theckman

gofumports is no longer a thing. Upstream now suggests to either let gopls handle the imports, or to run goimports followed by gofumpt (https://github.com/mvdan/gofumpt#frequently-asked-questions).

The former model doesn't require any changes by us, the latter would require a way to run both goimports and gofumpt and pass different flags to them.

I predict that most people will be using gopls in the future, so I'm not sure making this work is all too important in the long-term.

dominikh avatar Dec 28 '22 17:12 dominikh