search

dominikh / go-mode.el

Emacs mode for the Go programming language
BSD 3-Clause "New" or "Revised" License
1.37k stars 209 forks source link

Support for gofumports #354

Open theckman opened 4 years ago

theckman commented 4 years ago

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.

psanford commented 4 years ago

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)))
theckman commented 4 years ago

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.

dominikh commented 1 year ago

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.