Go: Add gotype support#1099
Conversation
| \ 'name': 'gotype', | ||
| \ 'output_stream': 'stderr', | ||
| \ 'executable': 'gotype', | ||
| \ 'command': 'gotype -e %t $(find $(dirname %s) -maxdepth 1 -name "*.go" | grep -v "^"%s"$")', |
There was a problem hiding this comment.
This command uses Bash syntax. This isn't portable.
Does this command accept files via stdin? Or do you need to pass all of the .go files to the command? ALE adds < TEMPORARY_FILENAME by default to the end of the command with the contents of the buffer.
|
It accepts a single file only on stdinn and then no other files on the
command line itself. However, to do correct type checking it needs all the
go files from a directory passed in.
I'm not sure what way is best to make it portable. It requires all files
from the checked files directory to make it work correctly, but without the
file itself otherwise you get duplicate definition errors. That's what the
my current solution does with some simple posix tools.
…On Wed, Nov 8, 2017, 18:23 w0rp ***@***.***> wrote:
***@***.**** requested changes on this pull request.
------------------------------
In ale_linters/go/gotype.vim
<#1099 (comment)>:
> @@ -0,0 +1,10 @@
+" Author: Jelte Fennema ***@***.***>
+" Description: gotype for Go files
+
+call ale#linter#Define('go', {
+\ 'name': 'gotype',
+\ 'output_stream': 'stderr',
+\ 'executable': 'gotype',
+\ 'command': 'gotype -e %t $(find $(dirname %s) -maxdepth 1 -name "*.go" | grep -v "^"%s"$")',
This command uses Bash syntax. This isn't portable.
Does this command accept files via stdin? Or do you need to pass all of
the .go files to the command? ALE adds < TEMPORARY_FILENAME by default to
the end of the command with the contents of the buffer.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#1099 (review)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABG8Jgod3zNhzO8ieGzVcm2pXyUYtpMaks5s0eORgaJpZM4QWuX->
.
|
|
You'll have to see if there's some portable way of making the linter check all of the files you need to check. What happens if you only pass just the one file you're editing instead? |
|
Add |
|
It works only if all types and functions used in the file are defined in
it. And regarding lint_file, yes that would work obviously, but I like type
checking to happen while typing. And with portable you mean make it work on
Windows, or make ik work with posix shell?
…On Wed, Nov 8, 2017, 19:01 w0rp ***@***.***> wrote:
Add 'lint_file': 1 to the Dictionary if you have to check files on disk.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#1099 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABG8JvdxbANZQu_ayArG88jTKpTbSbCGks5s0ex_gaJpZM4QWuX->
.
|
|
Would `bash -c 'command'` be portable?
…On Wed, Nov 8, 2017, 20:23 Jelte Fennema ***@***.***> wrote:
It works only if all types and functions used in the file are defined in
it. And regarding lint_file, yes that would work obviously, but I like type
checking to happen while typing. And with portable you mean make it work on
Windows, or make ik work with posix shell?
On Wed, Nov 8, 2017, 19:01 w0rp ***@***.***> wrote:
> Add 'lint_file': 1 to the Dictionary if you have to check files on disk.
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <#1099 (comment)>, or mute
> the thread
> <https://github.com/notifications/unsubscribe-auth/ABG8JvdxbANZQu_ayArG88jTKpTbSbCGks5s0ex_gaJpZM4QWuX->
> .
>
|
|
You can only check for errors while you type if the tool supports it. I don't know if |
|
Well, the command I made works fine. The errors show up when you type. So
I'm not sure what you mean with you can't. Do you mean if you change two
files and don't save both of them? Because that would indeed not work.
…On Thu, Nov 9, 2017, 00:20 w0rp ***@***.***> wrote:
You can only check for errors while you type if the tool supports it. I
don't know if gotype supports checking files via stdin with support for
imports.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#1099 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABG8JpBnWKuY7zd__766gcOQekUkgPDbks5s0jdGgaJpZM4QWuX->
.
|
|
And it does support stdin, it just doesn't allow passing in the other files
of the directory. Although the tool can be changed quite easily to allow
this (I have a simple proof of concept locally). However, it would still
require the other files from the directory to be passed in by filename.
Otherwise it's missing type data from those.
…On Thu, Nov 9, 2017, 00:24 Jelte Fennema ***@***.***> wrote:
Well, the command I made works fine. The errors show up when you type. So
I'm not sure what you mean with you can't. Do you mean if you change two
files and don't save both of them? Because that would indeed not work.
On Thu, Nov 9, 2017, 00:20 w0rp ***@***.***> wrote:
> You can only check for errors while you type if the tool supports it. I
> don't know if gotype supports checking files via stdin with support for
> imports.
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <#1099 (comment)>, or mute
> the thread
> <https://github.com/notifications/unsubscribe-auth/ABG8JpBnWKuY7zd__766gcOQekUkgPDbks5s0jdGgaJpZM4QWuX->
> .
>
|
|
Okay then. You'd just need to come up with something which works equally well on Linux, Mac, and Windows for ti to be considered for inclusion in this repository. |
|
Okay then. For now I'll just use my fork as it works perfectly on Linux and
Mac. But I'd like to to get this included, as it's really useful. I don't
know anything about Windows development environments though. Could you
answer some questions so I know what kind of solutions would be feasible:
- Can I assume any shell tools to be available?
- Can I make the linter require bash by using the "executable" field?
- Can I use shell expansion like, *.go?
- Can I somehow expand the full directory of the original file in the
command string? (e.g. By using %d)
…On Thu, Nov 9, 2017, 00:43 w0rp ***@***.***> wrote:
Okay then. You'd just need to come up with something which works equally
well on Linux, Mac, and Windows for ti to be considered for inclusion in
this repository.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#1099 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABG8Js4TCaH8fG4TGbVQ_NlEbNqUnIbMks5s0jyHgaJpZM4QWuX->
.
|
|
You can't rely on just about any shell features, including |
|
command_callback sounds like the way to go indeed. I'll take a look at that
when I have some time. Might take a while though because I'll be travelling
for a couple of weeks soon.
…On Thu, Nov 9, 2017, 01:15 w0rp ***@***.***> wrote:
You can't rely on just about any shell features, including *. You can
write a command_callback to implement all kinds of behavior. Have a look
at some of the existing linters.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#1099 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABG8JjjB0KBi2a1DEJPyWdst6LzT2EsAks5s0kQQgaJpZM4QWuX->
.
|
|
I just pushed a version where I converted my shell globbing and filtering to vim globbing and filtering by using |
w0rp
left a comment
w0rp
left a comment
There was a problem hiding this comment.
It it possible to use a directory name instead of a glob? Globbing can execute pretty slowly. The time frame for getting a command to run is around 16ms.
|
Not while also passing in the temporary file and it needs to exclude the
current file from the glob list, which is not possible if only a directory
is passed in.
…On Thu, Nov 9, 2017, 23:52 w0rp ***@***.***> wrote:
***@***.**** commented on this pull request.
It it possible to use a directory name instead of a glob? Globbing can
execute pretty slowly. The time frame for getting a command to run is
around 16ms.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#1099 (review)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABG8JqR4eeCuLYqGST10rT3XVPEM3gafks5s04ISgaJpZM4QWuX->
.
|
|
I recommend passing the actual filename and only checking for errors on save or enter then. That's how most of the other Go tools work. Nearly all Go tools don't support checking for errors via stdin. |
|
The only glob I'm doing is in a single directory (not recursive), so I cannot really imagine it to be very slow. When I use it, it doesn't feel slow at least. The argument, that that's how the other go tools work I don't really get. This improvement could maybe be used for them as well. |
w0rp
left a comment
w0rp
left a comment
There was a problem hiding this comment.
Let's go with it and see what others make of it. Can you add a Vader test for the new function?
|
I can, but I won't be able to do it before I'm travelling anymore. So I will probably do it somewhere in december (just letting you know). |
|
Okay, sounds good. |
|
Ok it took a while, but I finally wrote some tests. Any clue what the cause of the error on AppVeyor is? I don't define a variable |
|
I'm not sure. I'll have to run the tests on Windows myself and see if I can figure out what is wrong. |
|
Figured out the issue, I wasn't escaping a string correctly that was eval-ed as vimscript. Tests are passing now, so I think it can be merged now. |
|
Okay, let's go with it. |
|
Cheers! 🍻 |
|
Due to this issue, I had to make gotype only check files on disk: #1392 |
2 participants
No description provided.