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

[Feature Request] Integrate bradfitz/goimports #362

Closed
robfig opened this Issue Sep 8, 2013 · 34 comments

Comments

Projects
None yet
7 participants
@robfig
Copy link

robfig commented Sep 8, 2013

bradfitz/goimports is a replacement for gofmt that adds/removes standard library imports. I have been using it for a long time (from emacs) and it works great.

Might you consider integrating that, or is there an easy way to accomplish that replacement for end-users?

Thanks!

@DisposaBoy

This comment has been minimized.

Copy link
Owner

DisposaBoy commented Sep 9, 2013

I'm not sure I see the benefit in its current state. As far as I understood, it provides the following features

  • automatically add imports (from the stdlib)

you can already add imports by pressing ctrl+dot,ctrl+p and typing a few letters from the pkg name. I'd rather improve that workflow than do this automatically because the automation can get it wrong . #60 is one improvement. as-is #271 but that requires ST3

  • automatically remove import

this one's a part of #60 . as i said there, i'm not a huge fan of it but if go/types is ready then it can probably be implemented along with a setting to turn it on/off.

  • fmt

GS already does that

@robfig

This comment has been minimized.

Copy link

robfig commented Sep 9, 2013

The difference is I can write code and have it work without having to even think about whether or not I've used a new package in that code snippet or do things to add it or remove it.

Personally I think it's a huge feature. Anyway, I guess I'll take a look at the plugin and see if I can figure out how to do it as a separate installation step.

Thanks anyway

@robfig robfig closed this Sep 9, 2013

@DisposaBoy

This comment has been minimized.

Copy link
Owner

DisposaBoy commented Sep 9, 2013

well, it's up to you. I don't mind assisting with the integration. I've already done that, you can have a look at the https://github.com/DisposaBoy/GoSublime-next/blob/master/gscommands.py#L17

The basic steps are:

  • hooks into the on_pre_save event
    • it has to be done before the file is saved
    • it has to be blocking, which means the command has to be fast, otherwise it will block the ui. in ST3 you can use on_pre_save_async but all that does is move the blocking out of the ui thread, it still needs to be fast
    • ui freeze becomes noticeable for me at approx 200ms, which is one of the reasons why GS doesn't call external commands, it can get very slow if your disk isn't fast or something else on the system if slowing it down
  • you probably won't get away with updating the view after it's been saved because there will be a delay, so the user starts typing again and suddenly there's a dialog promoting to reload it
  • you probably won't get away with overwriting the view because it will cause the view to scroll away from where the user was . you can use the gspatch module https://github.com/DisposaBoy/GoSublime/blob/master/gosubl/gspatch.py to update the view

note

as always, if you use any code from GS, let me know in advance and we can discuss APIs etc. because you don't want me to delete code or move a module and break your code

@robfig

This comment has been minimized.

Copy link

robfig commented Sep 9, 2013

This guidance is very helpful!!! Thank you

For what it's worth, I was also planning on forking goimports to make it even more useful, like indexing your GOPATH so that it can automagically handle third-party imports as well, and perhaps allowing configuration on priority for ambiguous imports.

@DisposaBoy

This comment has been minimized.

Copy link
Owner

DisposaBoy commented Sep 9, 2013

If it helps, MarGo already has a method that sends the list of packages and their names... although the implementation is currently very innefficient

@fatih

This comment has been minimized.

Copy link

fatih commented Nov 4, 2013

@robfig did you made the changes? If yes I would like to test it.

@robfig

This comment has been minimized.

Copy link

robfig commented Nov 4, 2013

@ttacon did the integration. You can find it at https://github.com/ttacon/GoSublime (maybe he can chime in with installation steps)

@DisposaBoy

This comment has been minimized.

Copy link
Owner

DisposaBoy commented Nov 4, 2013

someone shoulda told me they were going to do that fork. from what i can see, all that was done was essentially call goimports on the file content, and unless i missed something, much pain could've been avoided by changing mg9.fmt to:

def fmt(fn, src):
    res, err = bcall('sh', {
        'Env': sh.env(),
        'Cmd': {
            'Name': 'goimports',
            'Input': src or '',
        },
    })
    return res.get('out', ''), err

as-is the use of sh.ShellCommand would've been better as sh.Command because the shell can output garbage and you in turn output it into the file and if the command fails, you probably won't know about it because the shell exits successfully

@fatih

This comment has been minimized.

Copy link

fatih commented Nov 5, 2013

@DisposaBoy do you mean I have to change the settings inside de gosublime package? what exactly are the steps? I really would like to try it.

@DisposaBoy

This comment has been minimized.

Copy link
Owner

DisposaBoy commented Nov 5, 2013

@fatih, there is no support for goimports in GS or GS-next. I was mostly just responding to @ttacon's fork. From the history, I can see that s/he suffered a bit of pain (and probably still didn't manage to maintain Windows support). But simply changing the fmt function inside of gosubl/mg9.py to the code I posted above would've achieved the same thing

@fatih

This comment has been minimized.

Copy link

fatih commented Nov 5, 2013

@DisposaBoy yeah, but then this would disable basically go-fmt right? I saw the the field "on_save": [], in settings but I think that doesn't do the thing I want. Maybe something like "before_save": [], which also modifies the file would be helpful. Goimports is really nice. I hope you consider to add it or add some convenient helper which would make it for us easier to implement.

@DisposaBoy

This comment has been minimized.

Copy link
Owner

DisposaBoy commented Nov 5, 2013

@fatih goimports does fmt as well so it's just trading one implementation for another. The only thing lost is that fmt no long follows your Go installation unless you make sure goimports is rebuilt when you update/downgrade/or otherwise switch Go installations. Support for this really needs to come from inside GS because:

  • on_save etc. settings (and their future replacements) are asynchronous, and fmt really needs to be blocking.
  • we can't replace the contents of the view whole-sale because it causes the view to jump around. that's why GS's fmt functionality does a diff and merges the new content into the view.

GS-next has support for richer commands and I imagine they could gain support for a blocking mode as well as support for updating the view, at which point GS could decide to do a diff/merge instead of a full replacement. I could then change the fmt function to run a pre-defined command which you can override as you see fit.

see https://github.com/DisposaBoy/GoSublime-next/blob/master/GoSublime.sublime-settings#L214 for documentation on the new commands.

... But it's not a feature that I have any use for right now so it's really up to you guys to propose integration ideas.

@DisposaBoy

This comment has been minimized.

Copy link
Owner

DisposaBoy commented Nov 16, 2013

no comment, so i went ahead and added a new setting fmt_cmd that i use to implement overriding margo's fmt . setting is documented here https://github.com/DisposaBoy/GoSublime-next/blob/master/GoSublime.sublime-settings#L52 ... it's only on gs-next so if you want to try it out just switch to that repo instead, it will eventually be pushed to the main repo... it seems to work fine, although for me it noticeably lags when it needs to insert an import statement

@fatih

This comment has been minimized.

Copy link

fatih commented Nov 18, 2013

Thanks @DisposaBoy I really appreciate it. It's a great command and some people I know already integrated this workflow.

@campoy

This comment has been minimized.

Copy link

campoy commented Dec 5, 2013

How do I use this feature?

I'm assuming I have to use GoSublime-next on my SublimeText instead of the standard GoSublime, but I don't see how to do that. Currently I'm using:

  • Sublime Text 2 Version 2.0.2, Build 2221
  • Latest version of GoSublime

And in my user preferences I set:

"fmt_cmd" : ["goimports"],

And still it doesn't work and no error is logged.

@mdwhatcott

This comment has been minimized.

Copy link

mdwhatcott commented Dec 6, 2013

I'm really looking forward to this. When will this feature be brought from gs-next into this repository?

@campoy

This comment has been minimized.

Copy link

campoy commented Dec 6, 2013

In the meanwhile I wrote a post on how I installed GoSublime-next and linked it to goimports.

http://blog.campoy.cat/2013/12/integrating-goimports-with-gosublime-on.html

I hope it helps

@DisposaBoy

This comment has been minimized.

Copy link
Owner

DisposaBoy commented Dec 12, 2013

@campoy an alternative, maybe easier way is to use git clone directly. git clone https://github.com/DisposaBoy/GoSublime-next.git ~/.config/sublime-text-3/Packages/GoSublime (adjusting the destination for Windows and OS X)

@DisposaBoy

This comment has been minimized.

Copy link
Owner

DisposaBoy commented Dec 12, 2013

@mdwhatcott I'll try and do it this weekend

@mdwhatcott

This comment has been minimized.

Copy link

mdwhatcott commented Dec 14, 2013

Works wonderfully! Thanks @DisposaBoy !

@DisposaBoy

This comment has been minimized.

Copy link
Owner

DisposaBoy commented Dec 15, 2013

@mdwhatcott just a heads-up:

never edit the default config file. It will be overwritten the next time GoSublime is updated and in some cases might even make the update fail. You should edit Packages/User/GoSublime.sublime-settings i.e ctrl+dot,ctrl+5 and super+dot,super+5 on OS X . This also applies to all other plugins, in general you should only edit files in Packages/User/

@mdwhatcott

This comment has been minimized.

Copy link

mdwhatcott commented Dec 16, 2013

Thanks @DisposaBoy, I've updated my instructions accordingly.

@dmitshur

This comment has been minimized.

Copy link

dmitshur commented Dec 16, 2013

Thanks! 👍 goimports is awesome.

@davidsonff

This comment has been minimized.

Copy link

davidsonff commented May 5, 2014

I can't seem to get this working on ST 3... There doesn't seem to be a "fmt_cmd" in any of the settings and if I add one it just disappears... Any thoughts?

@mdwhatcott

This comment has been minimized.

Copy link

mdwhatcott commented May 5, 2014

@davidsonff - Did you follow each step in this guide?

http://michaelwhatcott.com/gosublime-goimports/

If so, is goimports installed? Can you invoke goimports from the command line?

$GOPATH/bin/goimports

Also, you have to actually add the "fmt_cmd" setting in the user-specific gosublime preferences/config file. (⌘. ⌘5 is the shortcut chord on mac).

@davidsonff

This comment has been minimized.

Copy link

davidsonff commented May 5, 2014

Well, I'm on Windows... My file GoSublime.sublime-settings in my User directory is:

{
"fmt_cmd": ["goimports.exe"],
"env": {"GOPATH":"D:\Users\fdavidso\Documents\Go"}
}

I can execute goimports from the command line by just typing it since I added the various folders to my PATH variable. Also my Windows environmental variables are set:

C:\Users\fdavidso>set
ALLUSERSPROFILE=C:\ProgramData
...
GOPATH=D:\Users\fdavidso\Documents\Go
GOROOT=C:\Go
...

I have goimports.exe in both c:\Go\bin and D:\Users\fdavidso\Documents\Go\bin\

I think I've followed the instructions...

@mdwhatcott

This comment has been minimized.

Copy link

mdwhatcott commented May 5, 2014

Ah, windows. I'll have to try that.

Is %GOPATH%/bin in your %PATH%?

Maybe you could include the entire absolute path to the goimports:

{
"fmt_cmd": ["C:\blah\blah\goimports.exe"],
//etc...
}

I'll play around with my windows VM and see if I can get it working...

@davidsonff

This comment has been minimized.

Copy link

davidsonff commented May 6, 2014

No luck with putting the whole path into fmt_cmd, I'm afraid... Yes, I don't see any missing PATHs... Thanks for your any help or advice you are able to give!

@mdwhatcott

This comment has been minimized.

Copy link

mdwhatcott commented Jun 13, 2014

Any luck yet?

I just tried a fresh install of sublime text 3, installed package control, installed the GoSublime package with Package Control: Install Package, configured the GoSublime user preferences file (exactly has it is shows on my blog) and it worked.

I've got my $GOPATH/bin in my path.

@davidsonff

This comment has been minimized.

Copy link

davidsonff commented Jun 19, 2014

No happiness...

Here's my GoSublime.sublime-settings:

{
"fmt_cmd": ["goimports"]
}

GoSublime seems fine.

Here's the init info:

** 2014-06-19 13:56:07.568192 **:
GoSublime init r13.08.31-1 (0.003s)
| install margo: no
| install state: done
| sublime.version: 3059
| sublime.channel: stable
| about.ann: a13.09.07-1
| about.version: r13.08.31-1
| version: r13.08.31-1
| platform: windows-x64
| ~bin: D:\users\fdavidso\AppData\Roaming\Sublime Text
3\Packages\User\GoSublime\windows-x64\bin
| margo.exe: ~bin\gosublime.margo_r13.08.31-1_go1.3.exe (ok)
| go.exe: C:\Go\bin\go.exe (ok)
| go.version: go1.3
| GOROOT: C:\Go
| GOPATH: D:\Users\fdavidso\Documents\Go
| GOBIN: (not set) (should usually be (not set))
| set.shell: []
| env.shell:

| shell.cmd: ['C:\Windows\system32\cmd.exe', '/C', '${CMD}']

Any insights would be great!

Frank

On Fri, Jun 13, 2014 at 4:16 PM, Michael Whatcott notifications@github.com
wrote:

Any luck yet?

I just tried a fresh install of sublime text 3
http://www.sublimetext.com/3, installed package control
https://sublime.wbond.net/installation, installed the GoSublime package
with Package Control: Install Package, configured the GoSublime user
preferences file (exactly has it is shows on my blog) and it worked.

I've got my $GOPATH/bin in my path.


Reply to this email directly or view it on GitHub
#362 (comment)
.

Frank Davidson
Email: ffdavidson@gmail.com
Twitter: @davidsonff

@mdwhatcott

This comment has been minimized.

Copy link

mdwhatcott commented Jun 19, 2014

@davidsonff - Can you verify that the following path exists?

D:\Users\fdavidso\Documents\Go\bin\goimports.exe
@davidsonff

This comment has been minimized.

Copy link

davidsonff commented Jun 19, 2014

yup...

[image: Inline image 1]

On Thu, Jun 19, 2014 at 3:15 PM, Michael Whatcott notifications@github.com
wrote:

@davidsonff https://github.com/davidsonff - Can you verify that the
following path exists?

D:\Users\fdavidso\Documents\Go\bin\goimports.exe


Reply to this email directly or view it on GitHub
#362 (comment)
.

Frank Davidson
Email: ffdavidson@gmail.com
Twitter: @davidsonff

@mdwhatcott

This comment has been minimized.

Copy link

mdwhatcott commented Jun 19, 2014

I'm stumped.

@davidsonff

This comment has been minimized.

Copy link

davidsonff commented Jun 19, 2014

Cosmic rays, probably... Thanks for your efforts, though!

On Thu, Jun 19, 2014 at 5:07 PM, Michael Whatcott notifications@github.com
wrote:

I'm stumped.


Reply to this email directly or view it on GitHub
#362 (comment)
.

Frank Davidson
Email: ffdavidson@gmail.com
Twitter: @davidsonff

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment