-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
go.mod file support #1931
go.mod file support #1931
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work @fatih. It's exciting to see this support being added. I've left a few comments; most of them are either about possibly mistakenly added files or trying to be consistent with other approaches around error and list handling.
autoload/go/fix.vim
Outdated
@@ -0,0 +1 @@ | |||
/Users/fatih/go/src/github.com/fatih/goautofix/vim/fix.vim |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like it was mistakenly committed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it was symlinked (I was lazy) from a personal project for a small tool I wrote and wanted to try out. Removed them :)
autoload/go/mod.vim
Outdated
call go#list#Window(l:listtype, len(a:errors)) | ||
endfunction | ||
|
||
function! go#mod#ToggleModfmtAutoSave() abort |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fmt
should probably be Fmt
for consistency...
autoload/go/mod.vim
Outdated
let l:listtype = go#list#Type("GoModFmt") | ||
if !empty(a:errors) | ||
call go#list#Populate(l:listtype, a:errors, 'Format') | ||
echohl Error | echomsg "GoModFmt returned error" | echohl None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use go#util#EchoError
instead...
function! s:show_errors(errors) abort | ||
let l:listtype = go#list#Type("GoModFmt") | ||
if !empty(a:errors) | ||
call go#list#Populate(l:listtype, a:errors, 'Format') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of passing in the parsed errors and calling go#list#Populate
manually, can you use an errorformat
string and call go#list#ParseFormat
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just tried it, but I think it'll not work because we run the command on a temporary file first (because go mod edit
does an in-place edit). This means that the initial %f
is the temporary filename. This is copied from fmt.vim
and it's the same issue. I'll check if I can do something ( I would prefer ParseFormat
indeed)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmmm... If you know the temporary filename, then it should be easy enough to do a substitute(...)
on a:errors
before passing to ParseFormat
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think at this point, this is too much a hack. We're having a working version and it really works fine. Not sure if it's worth doing it (same applies for gofmt).
autoload/go/mod.vim
Outdated
" list of errors to be put into location list | ||
let errors = [] | ||
for line in splitted | ||
let tokens = matchlist(line, '^\(.\{-}\):\(\d\+\):\(\d\+\)\s*\(.*\)') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This regex is relatively easy to turn into an errorformat
string. It looks like it follows the usual pattern, so the errorformat
string would be %f:%l:%c:\ %m
ftplugin/go/commands-fix.vim
Outdated
@@ -0,0 +1 @@ | |||
/Users/fatih/go/src/github.com/fatih/goautofix/vim/commands-fix.vim |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this another mistakenly added file?
ftdetect/gofiletype.vim
Outdated
@@ -31,4 +31,8 @@ au BufReadPost *.s call s:gofiletype_post() | |||
|
|||
au BufRead,BufNewFile *.tmpl set filetype=gohtmltmpl | |||
|
|||
au BufNewFile *.mod setfiletype gomod | setlocal fileencoding=utf-8 fileformat=unix | |||
au BufRead *.mod call s:gofiletype_pre("gomod") | |||
au BufReadPost *.mod call s:gofiletype_post() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This conflicts with a built-in filetype: https://github.com/vim/vim/blob/master/runtime/filetype.vim#L996-L1002
Would probably be better to explicitly use go.mod
, and maybe even check if the first line matches module <ImportPath>
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch @Carpetsmoker !
go.mod
seems like the right call to me, maybe in conjunction with check the first line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add the checking line 👍 However, can I really use a .
inside a ft? I checked the link martin gave and a filetype is always a single word. I think we keep it gomod
. It's not that bad.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nevermind. I misunderstood, gonna use explicit
go.mod :)
This adds initial support for the `go.mod` file. It adds the followings: * Syntax highligthing. We highlight keywords, strings, operator and semver version. It works pretty well for now. * Auto fmt on save. Command `:GoModFmt` or `Plug(go-mod-fmt)` for custom mappings Before we fully support the semantics of go.mod, I think this initially will be helpful because I discovered that `go.mod` is read and edited a lot. So going forward, this will make it easier experimenting with Go modules. related: #1906
dd1dd81
to
3d725eb
Compare
@bhcleek @Carpetsmoker fixed all review comments and added a couple of improvements. PTAL |
ce4af06
to
d13e1e7
Compare
lgtm, but there's linting problems. |
It's just one line in the docs that's over the line limit (needs to be wrapped/rewritten). I'd fix it if you want (I don't have access though). |
* Fix auto toggle command * Add guard agains Go versions lower than 1.11 * Add documentation * Fixed setting correct filetype * Fixed parsing errors * Changed variable names to be consistent
d13e1e7
to
3b04072
Compare
@bhcleek @voutasaurus thanks! It was late after I pushed it, so didn't check the Travis results. I fixed and force pushed it. I'll merge it once it's green. |
This adds initial support for the
go.mod
file. It adds thefollowings:
semver version. It works pretty well for now.
:GoModFmt
orPlug(go-mod-fmt)
for custommappings
Before we fully support the semantics of go.mod, I think this initially
will be helpful because I discovered that
go.mod
is read and edited alot. So going forward, this will make it easier experimenting with Go
modules.
:GoModFmt
related: #1906