Breaking change https://github.com/fatih/vim-go/pull/1211

[fatih]

Should be reverted or fixed

[cespare]

I don't understand the issue.

If your go_fmt_command is set to goimports, then it makes sense that go_fmt_options would be goimports options.

In #1211 @hori-ryota wrote that he wants to use -s, and that doesn't work with goimports. But if you're using goimports instead of gofmt, why are you setting -s in go_fmt_options?

And if it's not OK to use go_fmt_options with goimports, why is it OK to use with some other command? For example what if I set go_fmt_command = "mytool", and mytool doesn't have -s either?

[cespare]

I think I understand now -- this is for using the explicit commands :GoFmt and :GoImports rather than the automatic gofmt-on-save.

Looking at this more, the situation is kind of messy. To summarize:

• :GoFmt runs go_fmt_command, which is user-overridable and defaults to "gofmt".
• :GoImports runs go_goimports_bin, which is user-overridable and defaults to "goimports".
• auto-fmt-on-save runs go_fmt_command.

It's strange that GoFmt and GoImports might run different commands than "gofmt" and "goimports". (For example, with my vim configuration, where I have go_fmt_command = "goimports", both :GoFmt and :GoImports run goimports. And for maximum confusion, I could even set go_goimports_bin = "gofmt"!) It seems to me that there should be one mechanism for selecting what formatting tool to use when auto-formatting and that shouldn't change the behavior of :GoFmt and :GoImports.

I don't think the proposed solution (adding go_imports_options and special-casing the string "goimports" in go#fmt#Format) is good because of the go_fmt_command = "mytool" case that I mentioned above.

TBH I'm not sure why :GoImports exists -- everything would be much simpler if there were only :GoFmt and go_fmt_command and then it would be unambiguous that if you set go_fmt_command = "goimports" then go_fmt_options means goimports options.

As a compromise, we could change go_fmt_options to be a dictionary:

let g:go_fmt_options = {
  \ 'gofmt': '-s',
  \ 'goimports': '-local mycompany.com',
  \ }

This doesn't help with the :Gofmt/:GoImports confusion though.

[hori-ryota]

Is it such an implementation that you mean?

  let opts = g:go_fmt_options
  if type(g:go_fmt_options) == type({})
    let opts = has_key(g:go_fmt_options, a:bin_name) ? g:go_fmt_options[a:bin_name] : ""
  endif
  call extend(cmd, split(opts, " "))

[fatih]

Just to chime in @cespare. The reason :GoImports exists is, that goimports is very slow on huge monolithic repos. I've worked on these repos and many others are still working (including me). So using :GoFmt and setting it to goimports means that every time you hit save (:w) there will be a slowness. So on way of dealing and improving this user experience is to set :GoFmt to use gofmt and then use the explicit :GoImports only in those cases when you want to run goimports. Does it makes sense for you? If you think the current situation can be improved I'm more than happy.

I agree that it's a little bis mess. I'll look later more in detail

[hori-ryota]

Is it wrong to revert once?