Add gofmt + goimports Support #9

joefitzgerald · 2014-03-06T05:24:12Z

This PR picks up where #3 left off by removing the snippets, and then ensuring goimports will work as a substitute for gofmt.

🆕 Support goimports
📝 Enable formatting on save by default
📝 Rename the menu item to "Format File"
💄 Remove tab options, which are no longer part of gofmt or goimports
🆕 Display gofmt errors, highlighting the line number(s) of the error(s)

Including because they're supported by both gofmt and goimports.

At one point that didn't work for me, but now, as if by magic, it does!

rubyist · 2014-03-06T14:48:49Z

Cool, I'm 👍 on merging this and nuking #3. I've been using this every day and it's been stable.

I did notice that this branch defaults the format on save to true, though there was some internal discussion about whether it should be on or off by default. Personally, I'm OK with it on by default, but will defer to the Atom team for that. @kevinsawicki any thoughts on that?

joefitzgerald · 2014-03-06T16:13:57Z

The rationale for enabling format on save by default:

GoSublime has it enabled by default
It's wonderfully useful
We should guide new users into the pit of success

A well-designed system makes it easy to do the right things and annoying (but not impossible) to do the wrong things)

darkhelmet · 2014-03-06T18:17:22Z

lib/gofmt.coffee

+
+  handleBufferEvents: (editor) ->
+    buffer = editor.getBuffer()
+    @subscribe buffer, 'saved', =>


buffer.on 'saved', => and you can get rid of {Subscriber} = require 'emissary'

Oh but then there's on unsubscribe...not sure if required...

The Atom folks told me this ensures proper clean up of things, I'll check with them again as I'm not entirely clear on the distinction and what the preferred way is.

Addressed in 560e30b - it's not required.

darkhelmet · 2014-03-06T18:41:19Z

🚢

- Future Enhancement: Highlight errors using information in stdout

joefitzgerald · 2014-03-07T16:49:43Z

@kevinsawicki @rubyist All done with changes on this feature branch. OK to merge?

rubyist · 2014-03-07T17:15:28Z

👍 from me, I'll defer to atom team to do the actual merging

wadey · 2014-03-07T18:57:32Z

👍, this branch is working great for me.

rosshale · 2014-03-08T21:42:33Z

👍 using this branch @ Go bootcamp. Works great!

- Shamelessly borrowed approach and code from [atom-jshint](https://github.com/Joezo/atom-jshint)

rubyist · 2014-03-09T20:00:28Z

Awww yissss. Thanks for doing that, it's the one thing I've been really missing over my emacs setup. 👍!

- We Are Currently Affected By http://discuss.atom.io/t/atom-command-doesnt-pass-environment-variables-to-atom/1596

joefitzgerald · 2014-03-09T21:29:25Z

I did some digging this morning, because @rubyist's commit (68c94b6) addressed touched on an issue I was intermittently experiencing also. I suspect that providing the current environment to packages is something the Atom team is trying to address / undoing / trying to address (which may explain why – "as if by magic" – things started working, and then broke again). As of 0.69.0 – by default – the app does not provide the current environment to packages.

For reference, the Pandoc package is experiencing similar issues:

I can confirm that I can reliably make things work with just gofmt as the value for language-go.gofmtPath so long as env is used to launch Atom (via the command line). gofmt is not found if I launch Atom via Finder, as the path is /usr/bin:/bin:/usr/sbin:/sbin:

language-go: error launching format command [gofmt] – Error: spawn ENOENT – current PATH: [/usr/bin:/bin:/usr/sbin:/sbin] gofmt.coffee:28
gofmtlanguage-go: format – exited with code [-2] gofmt.coffee:31

Launching Atom via env, and then using a nonexistent path for language-go.gofmtPath shows the difference in PATH when a package has the correct environment: /Users/jfitzgerald/.rbenv/shims:/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:/usr/local/go/bin:/usr/local/go/bin:/Users/jfitzgerald/go/bin:/Users/jfitzgerald/Projects/Packer:/usr/bin:/bin:/usr/sbin:/sbin:/usr/local/bin:/usr/local/go/bin:/usr/local/go/bin:/Users/jfitzgerald/go/bin

language-go: error launching format command [/path/that/is/nonexistent/gofmt] – Error: spawn ENOENT – current PATH: [/Users/jfitzgerald/.rbenv/shims:/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:/usr/local/go/bin:/usr/local/go/bin:/Users/jfitzgerald/go/bin:/Users/jfitzgerald/Projects/Packer:/usr/bin:/bin:/usr/sbin:/sbin:/usr/local/bin:/usr/local/go/bin:/usr/local/go/bin:/Users/jfitzgerald/go/bin] gofmt.coffee:28
/path/that/is/nonexistent/gofmtlanguage-go: format – exited with code [-2] gofmt.coffee:31

Based on this, it may be more sensible to specify /usr/local/go/bin/gofmt as the default value for language-go.gofmtPath. Thoughts?

mattetti · 2014-03-10T00:33:42Z

@joefitzgerald I just tested this PR using goimports and I don't see a warning due to compilation errors. Here is the code snippet I use:

package main

import "fmt"

func main() {
    fmt.Println("test")
    42
}

if I remove the fmt import statement, it gets re-added on save (as expected) but I don't see any errors related to the syntax error.

Note that I set the path manually (absolute path).

joefitzgerald · 2014-03-10T00:38:57Z

Note: gofmt is not the same as go build. It won't necessarily pick up compilation issues, but it will tell you when the source file is syntactically incorrect. Try adding a few exclamation marks inside main() (e.g. main(!)).

mattetti · 2014-03-10T01:41:06Z

hmm that's correct, I have vim setup to check for syntax errors every save, I really can't live without this feature, I'll check how vim does that and will get back to you.

joefitzgerald · 2014-03-10T01:44:43Z

Ultimately, if you have a .go file and run gofmt -w yourfile.go and gofmt prints nothing to stderr, you're not going to see anything in Atom.

elithrar · 2014-03-10T01:45:08Z

vim is likely using gocode, a third party package.

On Monday, March 10, 2014, Matt Aimonetti notifications@github.com wrote:

hmm that's correct, I have vim setup to check for syntax errors every
save, I really can't live without this feature, I'll check how vim does
that and will get back to you.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/9#issuecomment-37147446
.

mattetti · 2014-03-10T02:00:57Z

so after further investigation, I'm getting the syntax errors from syntastic https://github.com/scrooloose/syntastic which has support for go using the following files:
https://github.com/scrooloose/syntastic/tree/master/syntax_checkers/go

From what I can see, it is using go build/test:
https://github.com/scrooloose/syntastic/blob/master/syntax_checkers/go/go.vim#L51-L57

It also seem to support golint https://github.com/golang/lint

I don't think that syntax verification should be blocking this merge, but I think that atom should should have a way to (maybe optionally) check that your code's syntax is correct.

joefitzgerald · 2014-03-10T02:34:58Z

I think there is a general desire (and I personally share this) to also integrate gocode (see #2) and potentially other tools to round out the feature set here. I use GoSublime; others use vim and emacs. But I think we want the same set of features. This PR solely addresses gofmt (and substitutes, like goimports) integration, and so is limited by the limitations of those tools.

I've already created an issue in the autocomplete package repo to request additional extensibility features, in advance of gocode integration.

Your suggestion to leverage tools to highlight compile errors is a good one and we should create a new issue for it. It raises questions about $GOROOT, $GOPATH, and package root which could increase the complexity of integration. gofmt and goimports have the wonderful simplicity of operating on a single file which makes this integration easier to complete.

mattetti · 2014-03-10T02:41:23Z

I agree, but I also think that adding support for go build on save should
be pretty trivial and won't require 3rd party binaries. I am also looking
forward to seeing gocode and godef (
http://godoc.org/code.google.com/p/rog-go/exp/cmd/godef ) eventually
supported.

Dealing with $GOPATH might be interesting if we try to cover edge cases or
automate too much, but one thing for I've learned the hard way, don't set
$GOROOT anymore, it's a recipe for disaster.

On Sun, Mar 9, 2014 at 7:34 PM, Joe Fitzgerald notifications@github.comwrote:

I think there is a general desire (and I personally share this) to also
integrate gocode (see #2 #2)
and potentially other tools to round out the feature set here. I use
GoSublime; others use vim and emacs. But I think we want the same set of
features. This PR solely addresses gofmt (and substitutes, like goimports)
integration, and so is limited by the limitations of those tools.

I've already created an issuehttps://github.com/atom/autocomplete/issues/18in the autocomplete package repo to request additional extensibility
features, in advance of gocode integration.

Your suggestion to leverage tools to highlight compile errors is a good
one and we should create a new issue for it. It raises questions about
$GOROOT, $GOPATH, and package root which could increase the complexity of
integration. gofmt and goimports have the wonderful simplicity of
operating on a single file which makes this integration easier to complete.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/9#issuecomment-37149032
.

joefitzgerald · 2014-03-10T02:57:19Z

@probablycorey Notification that this refers to an issue you created against Atom.

joefitzgerald · 2014-03-11T03:13:43Z

Hmm... @rubyist any way to get attention on this? Would love to get this merged or critiqued and then build upon it.

mattetti · 2014-03-11T04:03:22Z

lib/gofmt.coffee

+      return if not matchLine?
+      error = [matchLine[2], matchLine[3], matchLine[4]]
+      errors.push error
+    loop


why using a loop here? I had a hard time understanding the logic flow between the loop, the extraction, error pushing and patching.

Regular expression evaluation (pattern.exec(data)) must be repeated until there are no more matches in order to collect all matches for a multi-line body of text. The loop continues until there are no more matches (we have reached the end of the error text).

mattetti · 2014-03-11T04:08:25Z

package.json

@@ -1,7 +1,8 @@
 {
  "name": "language-go",
  "description": "Go language support in Atom",
-  "version": "0.7.0",
+  "version": "0.8.0",


I'm not sure what the package maintainer policy is, but usually, PRs shouldn't include version upgrades so the author can choose when the version change takes place.

joefitzgerald · 2014-03-13T19:04:20Z

@kevinsawicki Happy to make changes or act on any feedback you have about this PR.

joefitzgerald · 2014-03-13T19:21:13Z

Folks, given the answer in #2 (comment), I'm going to close this, and pick up work in https://github.com/joefitzgerald/go-plus. I am open to renaming the package - just throwing something out there so we can rally around it.

The current scope of the language-* packages has been syntax highlighting, snippets, and language properties such as folding and indent patterns

I invite @mattetti, @rubyist, @rosshale, @darkhelmet, @apires, @drnic and others to collaborate with me there to add go build, gocode, gofmt, and other features in one package.

mattetti · 2014-03-13T19:26:20Z

hmm I think there is a compromise to be found, I'm afraid that with only "syntax highlighting, snippets, and language properties such as folding and indent patterns" the default go package isn't very useful. Due to the compiled nature of the language, I feel that a compilation/syntax check + reformatting of the code as per gofmt standards is really needed. Anything else, including godef, gocode etc.. should indeed live in another package.

My understanding from @kevinsawicki's comment wasn't that he didn't want to add extra features, but to keep it as minimal as possible. @kevinsawicki can you maybe explain what you had you mind or what you'd prefer us to do?

joefitzgerald · 2014-03-13T21:17:04Z

This is now published as package go-plus:

rubyist and others added 11 commits March 5, 2014 21:09

run gofmt on save

d31f0ca

Make formatting on save optional and off by default

ca89a87

Add gofmt as a command

f966775

for those who like to menu

5c0ebb7

saved is what we want here

9bb05d0

Provide options for using tab indentation and tab width

bf8e901

Including because they're supported by both gofmt and goimports.

Default to just 'gofmt' instead of a full path.

68c94b6

At one point that didn't work for me, but now, as if by magic, it does!

Fix Merge Issue, Ignore node_modules

a8df918

Add goimports Support, bump package version

28c6c66

Remove Tab Options (@darkhelmet Suggestion in #3)

de666fa

Ensure Menu Command Works

b94669a

joefitzgerald mentioned this pull request Mar 6, 2014

Adding support for syntax verification, gofmt / goimports #1

Closed

Removed goimports code path, after discussion with @rubyist in #9

94d32b0

rubyist mentioned this pull request Mar 6, 2014

run gofmt on save #3

Closed

4 tasks

darkhelmet reviewed Mar 6, 2014
View reviewed changes

joefitzgerald added 3 commits March 6, 2014 11:23

Remove explicit dependency on emissary

560e30b

💄 Return on same line

f64f2c5

💄 true is implicit, and thus redundant

0b112c2

Add rudimentary error handling and logging

f593602

- Future Enhancement: Highlight errors using information in stdout

🆕 Display gofmt error details and highlight rows with errors

8b995b5

- Shamelessly borrowed approach and code from [atom-jshint](https://github.com/Joezo/atom-jshint)

Clean Up Logging, Log PATH If Format Command Not Found

1dccd38

- We Are Currently Affected By http://discuss.atom.io/t/atom-command-doesnt-pass-environment-variables-to-atom/1596

Specify Absolute Path To gofmt As Default

cfadec7

joefitzgerald mentioned this pull request Mar 10, 2014

go build Integration #10

Closed

mattetti mentioned this pull request Mar 10, 2014

Compile on save + report syntax errors #11

Closed

mattetti reviewed Mar 11, 2014
View reviewed changes

if not -> unless

d825948

mattetti reviewed Mar 11, 2014
View reviewed changes

joefitzgerald added 2 commits March 10, 2014 23:15

💄 Move appends into loop

69b93c5

Move colon out of first match group.

6d5ac57

joefitzgerald closed this Mar 13, 2014

joefitzgerald deleted the gofmt-hooks-no-snippets branch March 13, 2014 21:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add gofmt + goimports Support #9

Add gofmt + goimports Support #9

joefitzgerald commented Mar 6, 2014

rubyist commented Mar 6, 2014

joefitzgerald commented Mar 6, 2014

darkhelmet Mar 6, 2014

darkhelmet Mar 6, 2014

rubyist Mar 6, 2014

joefitzgerald Mar 6, 2014

darkhelmet commented Mar 6, 2014

joefitzgerald commented Mar 7, 2014

rubyist commented Mar 7, 2014

wadey commented Mar 7, 2014

rosshale commented Mar 8, 2014

rubyist commented Mar 9, 2014

joefitzgerald commented Mar 9, 2014

mattetti commented Mar 10, 2014

joefitzgerald commented Mar 10, 2014

mattetti commented Mar 10, 2014

joefitzgerald commented Mar 10, 2014

elithrar commented Mar 10, 2014

mattetti commented Mar 10, 2014

joefitzgerald commented Mar 10, 2014

mattetti commented Mar 10, 2014

joefitzgerald commented Mar 10, 2014

joefitzgerald commented Mar 11, 2014

mattetti Mar 11, 2014

joefitzgerald Mar 11, 2014

mattetti Mar 11, 2014

joefitzgerald commented Mar 13, 2014

joefitzgerald commented Mar 13, 2014

mattetti commented Mar 13, 2014

joefitzgerald commented Mar 13, 2014

Add gofmt + goimports Support #9

Add gofmt + goimports Support #9

Conversation

joefitzgerald commented Mar 6, 2014

rubyist commented Mar 6, 2014

joefitzgerald commented Mar 6, 2014

darkhelmet Mar 6, 2014

Choose a reason for hiding this comment

darkhelmet Mar 6, 2014

Choose a reason for hiding this comment

rubyist Mar 6, 2014

Choose a reason for hiding this comment

joefitzgerald Mar 6, 2014

Choose a reason for hiding this comment

darkhelmet commented Mar 6, 2014

joefitzgerald commented Mar 7, 2014

rubyist commented Mar 7, 2014

wadey commented Mar 7, 2014

rosshale commented Mar 8, 2014

rubyist commented Mar 9, 2014

joefitzgerald commented Mar 9, 2014

mattetti commented Mar 10, 2014

joefitzgerald commented Mar 10, 2014

mattetti commented Mar 10, 2014

joefitzgerald commented Mar 10, 2014

elithrar commented Mar 10, 2014

mattetti commented Mar 10, 2014

joefitzgerald commented Mar 10, 2014

mattetti commented Mar 10, 2014

joefitzgerald commented Mar 10, 2014

joefitzgerald commented Mar 11, 2014

mattetti Mar 11, 2014

Choose a reason for hiding this comment

joefitzgerald Mar 11, 2014

Choose a reason for hiding this comment

mattetti Mar 11, 2014

Choose a reason for hiding this comment

joefitzgerald commented Mar 13, 2014

joefitzgerald commented Mar 13, 2014

mattetti commented Mar 13, 2014

joefitzgerald commented Mar 13, 2014