Skip to content

gofmt fixer for Go#970

Merged
w0rp merged 5 commits into
dense-analysis:masterfrom
aliou:go-fmt-fixer
Oct 3, 2017
Merged

gofmt fixer for Go#970
w0rp merged 5 commits into
dense-analysis:masterfrom
aliou:go-fmt-fixer

Conversation

@aliou

@aliou aliou commented Oct 1, 2017

Copy link
Copy Markdown
Contributor

Hello, this PR adds gofmt as a fixer for Go.

@w0rp w0rp left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to add entries to the README and the help file.

Have a look at whatever Travis is saying.

Comment thread autoload/ale/fixers/gofmt.vim Outdated

return {
\ 'command': ale#Escape(l:executable)
\ . '-l -w'

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're missing a space before -l here.

AssertEqual
\ {
\ 'read_temporary_file': 1,
\ 'command': ale#Escape(g:ale_go_gofmt_executable)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use a string literal for the executable here instead.

AssertEqual
\ {
\ 'read_temporary_file': 1,
\ 'command': ale#Escape(g:ale_go_gofmt_executable)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use a string literal for the executable here too.

\ {
\ 'read_temporary_file': 1,
\ 'command': ale#Escape(g:ale_go_gofmt_executable)
\ . '-l -w'

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You missed the space in the test too.

@aliou

aliou commented Oct 2, 2017

Copy link
Copy Markdown
Contributor Author

Updated with your review. I didn’t add gofmt to the readme or the help files because it is already there as a linter. Should I mention that it also works as a fixer?

@w0rp

w0rp commented Oct 2, 2017

Copy link
Copy Markdown
Member

Nah, that's okay. I didn't realise it was there.

Comment thread doc/ale-go.txt Outdated
number of linters known to be slow or consume a lot of resources.


===============================================================================

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check out the Travis CI results. It's complaining because there's no table of contents entry which references this. You should add gofmt to the table of contents.

@w0rp

w0rp commented Oct 2, 2017

Copy link
Copy Markdown
Member

Now Travis is complaining because gofmt has to come before gometalinter in the table of contents and also in the headings, because everything there is sorted lexicographically. (alphabetically)

@aliou

aliou commented Oct 2, 2017

Copy link
Copy Markdown
Contributor Author

So that's what I did first and then again in the last commit, but Travis is still failing. Do you know something I might have missed on this?

On a somewhat related note, I didn’t have these test fail locally because it seems like macOS’s version of paste (used in test/script/check-tock) doesn’t seem to work when passing data from STDIN and only seems to take a file. What do you think about also running this test in Docker?

Related line :
https://github.com/w0rp/ale/blob/e0bd490ed9150c8a229f127dcacbcbf97c9f9861/test/script/check-toc#L30

@w0rp

w0rp commented Oct 3, 2017

Copy link
Copy Markdown
Member

The script is doing the right thing. You need to take your entire gofmt section that you added to the Go documentation, and then move that above the gometalinter section. One part of the script checks that the table of contents is sorted, and the other checks that the headings and table of contents are in the same order.

@w0rp

w0rp commented Oct 3, 2017

Copy link
Copy Markdown
Member

I wouldn't expect those scripts to run on Mac, because Macs use an ancient version of Bash and BSD userland stuff. I might be able to make them compatible some day, at the cost of some sanity, or just get them to run in Docker.

@aliou

aliou commented Oct 3, 2017

Copy link
Copy Markdown
Contributor Author

Got it. Updated both the TOC and the order in the help files, and squash the relevant commits. LMK if something else is needed.

@w0rp w0rp left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is all fine and dandy. Thank you for your work on this. 👍

@w0rp w0rp merged commit e376f0a into dense-analysis:master Oct 3, 2017
@w0rp

w0rp commented Oct 3, 2017

Copy link
Copy Markdown
Member

Cheers! 🍻

@aliou

aliou commented Oct 3, 2017

Copy link
Copy Markdown
Contributor Author

Cool 🎉 thanks!

@aliou aliou deleted the go-fmt-fixer branch October 3, 2017 19:01
rsrchboy added a commit to rsrchboy/ale that referenced this pull request Oct 7, 2017
* upstream/master:
  Get more command callback tests to pass on Windows
  Add all possible config files for prettier
  gofmt fixer for Go (dense-analysis#970)
  Fix dense-analysis#964 - Remove signs when multiple signs end up on a single line
  Fix typos
  Get tslint and xmllint command callback tests to pass in Windows
  Add a Scripts dir for tests on Windows
  Get fixer tests to work on Windows
  Use local versions of yapf on Windows, and get the callback tests to pass
  Fix Flow and Idris tests for Windows
  Get the mcsc handler tests to pass on Windows
  Added g:ale_php_phpstan_configuration option
  Fix an issue with the check-supported-tools-tables script
  Implemented review recommendations
  Added advanced c-sharp linter
  Added advanced c-sharp linter
  There seems to be a bug in eslint that causes the `--config` option to not detect node_modules correctly. The `-c` option, however, works fine.
  Move dialect setting before user options (shellcheck)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants