Support neovim/neovim@a225374 api

[zchee]

Support neovim/neovim@a225374 API.

compare is here:

> nvim_buf_attach(buffer Buffer, send_buffer Boolean, opts Dictionary) Boolean { name(nvim_buf_attach) }
> nvim_buf_detach(buffer Buffer) Boolean { name(nvim_buf_detach) }
> nvim_buf_get_commands(buffer Buffer, opts Dictionary) Dictionary { name(nvim_buf_get_commands) }
> nvim_buf_is_loaded(buffer Buffer) Boolean { name(nvim_buf_is_loaded) }
> nvim_call_dict_function(dict Object, fn String, args Array) Object { name(nvim_call_dict_function) }
> nvim_get_chan_info(chan Integer) Dictionary { name(nvim_get_chan_info) }
> nvim_get_commands(opts Dictionary) Dictionary { name(nvim_get_commands) }
> nvim_get_proc(pid Integer) Object { name(nvim_get_proc) }
> nvim_get_proc_children(pid Integer) Array { name(nvim_get_proc_children) }
> nvim_list_chans() Array { name(nvim_list_chans) }
> nvim_list_uis() Array { name(nvim_list_uis) }
> nvim_parse_expression(expr String, flags String, highlight Boolean) Dictionary { name(nvim_parse_expression) }
> nvim_set_client_info(name String, version Dictionary, type String, methods Dictionary, attributes Dictionary) void { name(nvim_set_client_info) }
----
< nvim_get_color_map() map[string]int { name(nvim_get_color_map) }
> nvim_get_color_map() Dictionary { name(nvim_get_color_map) }

[zchee]

/cc: @garyburd

[zchee]

CI is failed :( I'll check it and fix later.

@justinmk BTW, Who is current neovim/go-client owner? still at garyburd?

[justinmk]

@zchee yes

[zchee]

@justinmk OK, So I want to the repository owner to rerun the CI because of TravisCI failed caused by wrong(mysterious?) error on go1.10.x.

• https://travis-ci.org/neovim/go-client/builds/412289316
• https://travis-ci.org/neovim/go-client/jobs/412289320 The error log is here:

No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself.
Check the details on how to adjust your build configuration on: https://docs.travis-ci.com/user/common-build-problems/#Build-times-out-because-no-output-was-received
The build has been terminated

Or, Should I just empty git amend and force push?

[zchee]

/ping @garyburd

[garyburd]

Thank you for the ping. I was traveling with limited internet access, and forgot to look at this when I did get access. I'll look at it tomorrow.

[zchee]

@garyburd Woa! where did you travel? :D And thanks, looks forward your review.

FYI, I said before,

Can I write additional type use iota?

It means I can write completely response struct, but might be needs many type Foo int and const Bar Foo = 1<iota and corespondents structs.

I want to ask to you whether the needs above implements or not. (in other works, you like or not)

[garyburd]

• The type interface{} should be avoided in the API. It's usually the case that interface{} can be eliminated by defining a struct type.
• Return "large" values by pointer. Channel is large.
• Regarding enumerated types: The precedent is to use strings. Defining types as you suggest adds some amount of type safety, but it does not make the API easier to use in a significant way.

I've been backpacking, camping and hiking in Washington & Wyoming.

[zchee]

@garyburd Sorry for the late reply :( I got it! will fix.

[zchee]

@garyburd Added Client struct and fixed some point. PTAL.

[zchee]

/ping @garyburd @justinmk

[zchee]

sorry, re-ping: @garyburd @justinmk

[justinmk]

@zchee I'm not qualified to review this, so we just have to wait. If we get more activity here perhaps @garyburd will add another maintainer.

[garyburd]

My feedback on the use of interface{} still stands. Values with type interface{} are much more difficult to pick apart than specific map or struct types. One example is that BufferCommands should return map of structs.

I have more time to work on this now. Do you want me to continue working on it?

[zchee]

@garyburd

My feedback on the use of interface{} still stands. Values with type interface{} are much more difficult to pick apart than specific map or struct types. One example is that BufferCommands should return map of structs.

I misunderstood your feedback. It means adding some new API return value to should not use interface as much as possible? If so, I’ll continue to fix this PR.

I have more time to work on this now. Do you want me to continue working on it?

If you have time, I glad to send PR to my PR.

BTW, neovim has been nvim_buf_set_virtual_text new api. I also want to use it, so will send another PR which supports this API. https://github.com/neovim/neovim/commit/45f53b370b11626468c35b58b939dfd2f0aa9de0

[garyburd]

I apologize for any confusion. Method return types should not include the type interface{} unless a value can be of multiple concrete types.

The BufferVar and Eval methods are examples of where interface{} in the return type is acceptable. The concrete type returned by these methods can be integer, string, map and other types.

The code generator has a special case for a return value of interface{}: the result is returned through a pointer argument instead of as a return value. This allows the application to specify a specific concrete type to MsgPack decoder.

The result of all this is that the application can work with concrete types instead of picking part results with type assertions.

[zchee]

@garyburd Thanks polite reply. I'll read your comment and will fix this pull request :)

[zchee]

@garyburd I was many changed and push new commit. If my interpretation is wrong, sorry take your time... But removed almost interface value.

PTAL.

[zchee]

@garyburd /cc @justinmk Sorry for late. I'll fix failed CI points later. And, If @garyburd is busy or no reply (I'll wait to ~1 week), I'll merge this pull request into master because now pending https://github.com/neovim/go-client/pull/40 If you don't like current interface variable, could you advise to me? (Anytime is fine. I will fix it ASAP.)

---

Also, I think time is come the go-client should release the versions. It is for make compatibility to more explicit with neovim core API. What do you think about it?

[justinmk]

Also, I think time is come the go-client should release the versions. It is for make compatibility to more explicit with neovim core API.

Not sure I follow. Nvim API is backwards-compatible, so go-client has no need to make breaking changes.

[zchee]

@justinmk

Nvim API is backwards-compatible, so go-client has no need to make breaking changes.

Ah, sorry for lack description. I mean, for example,

Release neovim/go-client@1.0.0 This version supported nvim_buf_set_virtual_text API (neovim/neovim@0ce8800)

like the neovim/pynvim, go-client also publish release note or etc, and describe what supported neovim upstream API. It's more easy to understand to developer who uses go-client.

Also, The Go language has been supported semantic version vendoring in the core runtime (still ALPHA yet, will stable on Go1.12). Now Go package authors preparing it gradually.

More details: https://github.com/golang/go/wiki/Modules#gomod

---

But yes, As your said, neovim core api have backward compatibility. I'm just suggested based on the current state of Go language. Versioning policy is up to you.

[garyburd]

The method BufferCommands is defined as:

 func BufferCommands(buffer Buffer, opts map[string]interface{}) map[string]interface{}

Each application that uses this method must use type assertions to access command properties. This work can be eliminated for all applications by declaring a command type:

type Command struct {
    Name string `msgpack:"name"`
    Definition string `msgpack:"definition"`
    ScriptID int `msgpack:"script_id"`
    ... and so on
}

 func BufferCommands(buffer Buffer, opts map[string]interface{}) map[string]*Command

If the PR is merged as is, then it's ugly moving forward to using a declared type for commands. The addition of the type requires either a breaking change or the addition of the second method for nvim_buf_get_commands.

 func BufferCommands(buffer Buffer, opts map[string]interface{}) map[string]interface{}
 func BufferCommands2(buffer Buffer, opts map[string]interface{}) map[string]*Command

I think the following are preferable to the current PR:

1. Declare and use a type for commands. This is the best option.
2. Omit the method from the PR. This is the least amount of work.
3. Declare an empty struct type for command with TODO comment stating that fields should be added. This is almost the same as omitting the method in terms of work and functionality, but it does leave a pointer forward for people who come looking for the method.

[garyburd]

It is more conventional and more convenient to declare types like Version as a struct:

type Version struct {
     Major int `msgpack:"major"`
     Minor int `msgpack:"minor"`
     ... and so on
}

Use "omitempty" as needed to omit optional values. If the zero value (typically the empty value) has meaning, then use pointer to value or "empty" field tag to omit the value.

[zchee]

@garyburd thanks polite reply. I'll read it.

[zchee]

@garyburd At first, Sorry for late reply :(

But I'd added Command type and fixed {Buffer}Commands return map type. Also changed the Version type to struct instead of map. PTAL.

---

BTW, are you busy these days...? Do you have time to review this PR?

[zchee]

@garyburd CI was failed.

• https://travis-ci.org/neovim/go-client/jobs/501637406

Here is the result:

.
.
ok      github.com/neovim/go-client/msgpack 1.087s
.
.
ok      github.com/neovim/go-client/msgpack/rpc 1.034s

No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself.
Check the details on how to adjust your build configuration on: https://docs.travis-ci.com/user/common-build-problems/#Build-times-out-because-no-output-was-received
The build has been terminated

It seems to relate to changed the nvim default behavior...? Anyway, the nvim packages testcase has stopped somewhere. I'll debug and fix it on another branch.

[zchee]

@garyburd rebased #41. CI is passed.

---

@justinmk Hmm, garyburd is maybe busy... 😱 I was fixed the he pointed out the problem. And I want to merge this pull request and start work on creating a new PR that supports Neovim latest API. Currently, the following API are not implemented yet. The following list contains useful APIs, such as nvim_buf_set_virtual_text, nvim_create_buf, and I want to use them.

So, I would like to merge if not reply for a few days(1~2 days) from him. What do you think?

---

> nvim_buf_clear_namespace(buffer Buffer, ns_id Integer, line_start Integer, line_end Integer) void { name(nvim_buf_clear_namespace) }
> nvim_buf_get_offset(buffer Buffer, index Integer) Integer { name(nvim_buf_get_offset) }
> nvim_buf_set_virtual_text(buffer Buffer, ns_id Integer, line Integer, chunks Array, opts Dictionary) Integer { name(nvim_buf_set_virtual_text) }
> nvim_create_buf(listed Boolean, scratch Boolean) Buffer { name(nvim_create_buf) }
> nvim_create_namespace(name String) Integer { name(nvim_create_namespace) }
> nvim_get_namespaces() Dictionary { name(nvim_get_namespaces) }
> nvim_input_mouse(button String, action String, modifier String, grid Integer, row Integer, col Integer) void { name(nvim_input_mouse) }
> nvim_open_win(buffer Buffer, enter Boolean, width Integer, height Integer, options Dictionary) Window { name(nvim_open_win) }
> nvim_select_popupmenu_item(item Integer, insert Boolean, finish Boolean, opts Dictionary) void { name(nvim_select_popupmenu_item) }
> nvim_set_vvar(name String, value Object) void { name(nvim_set_vvar) }
> nvim_ui_try_resize_grid(grid Integer, width Integer, height Integer) void { name(nvim_ui_try_resize_grid) }
> nvim_win_close(window Window, force Boolean) void { name(nvim_win_close) }
> nvim_win_config(window Window, width Integer, height Integer, options Dictionary) void { name(nvim_win_config) }
> nvim_win_set_buf(window Window, buffer Buffer) void { name(nvim_win_set_buf) }

[justinmk]

@zchee Thanks for your continued efforts!

Would it make sense to add a couple small tests for "sanity checking"? I think that not many tests are needed, but at least nvim_buf_attach was a bit tricky for other clients (such as https://github.com/neovim/node-client).

Also (perhaps in another PR) I think go-client should call nvim_set_client_info when connecting to set default channel info.

[zchee]

@justinmk

Would it make senes to add a couple small tests for "sanity checking"?

Got it. make sense :) I'll adding testcase.

Also (perhaps in another PR) I think go-client should call nvim_set_client_info when connecting to set default channel info.

It means, tells to nvim that information about go-client itself? Or, should forcing send nvim_set_client_info to clients used go-client, like zchee/nvim-go?

Anyway, If force, It would be good to implement it into these functions.

• https://github.com/neovim/go-client/blob/177c72b37553a0baf2bdc607a224e3dd768b99a8/nvim/nvim.go#L64-L68
• https://github.com/neovim/go-client/blob/177c72b37553a0baf2bdc607a224e3dd768b99a8/nvim/plugin/main.go#L48

---

Edit: Ah, overlooked this document.

This could happen if a library first identifies the channel, and a plugin using that library later overrides that info

So, as a result, We should implements go-client send nvim_set_client_info when connecting the nvim, but plugins can also sending that API with override, right?

[zchee]

@justinmk FYI, I was created implements neovim/neovim@6eca56c APIs branch. Still WIP.

• https://github.com/neovim/go-client@6eca56c
• commit: https://github.com/neovim/go-client/commit/cb3aeebc5123d5a1e610106aac2b8a7f79abd012

---

Also, Maybe we should adding more testcases in the future for new APIs. I'd like start to using codecov.io to go-client. like neovim/node-client.

[zchee]

@justinmk BTW, I found different implementation and document on neovim core nvim_win_close. That API void function, but comment document describe

@return Window number

• https://github.com/neovim/neovim/blob/master/src/nvim/api/window.c#L485

I was implemented non return variable on

• https://github.com/neovim/go-client/commit/cb3aeebc5123d5a1e610106aac2b8a7f79abd012#diff-76bd1623efc88af3adf88f49df257af3R761

But which is correct?

[justinmk]

So, as a result, We should implements go-client send nvim_set_client_info when connecting the nvim, but plugins can also sending that API with override, right?

Exactly.

Also, Maybe we should adding more testcases in the future for new APIs. I'd like start to using codecov.io to go-client. like neovim/node-client.

I don't think this client necessarily needs high test coverage. Most of the tests would be redundant with Nvim's tests. Hopefully go-client will stay simple so it won't need many tests. But some functionality such as nvim_buf_attach is worth testing.

BTW, I found different implementation and document on neovim core nvim_win_close.

The doc is wrong. Fixed in https://github.com/neovim/neovim/pull/9600 .

[daskol]

How much work is left? Can I help somehow? Current implementation support only API level 3. It is obsolete!

[garyburd]

Dictionaries with a known set of string keys should be mapped to a Go struct type. A struct carries more type information than a map[string]interface{} or other map type. The extra type information helps to reduce bugs and is a convenience when working with values returned from Nvim.

The methods argument to nvim_set_client_info is such a type.

Pointers to structs should be used for "large" structs. The definition of "large" is a matter of judgement. The Version type in this PR is what I would call large.

Given this, the SetClientInfo API should probably look like this:

func SetClientInfo(name string, version *Version, typ string, methods map[string]*Method, attributes Attributes) {
    name(nvim_set_client_info)
}

type Method  struct {
     Async bool `msgpack:"async"`
     NArgs MethodNArgs `msgpack:"nargs"`
}

type MethodNArgs struct {
      Min, Max int `msgpack:",array"`
}

[zchee]

@garyburd thanks for review. fix asap

[garyburd]

I have some comments on the following types used in the attributes argument to nvim_set_client_info:

type AttributesType string
type Attributes map[AttributesType]string

The values are strings, but I don't see a corresponding restriction in the Neovim code. Should the type interface{} be used for the value?

The key type, AttributesType, does not carry its weight. The type adds to the API surface area, but it does not prevent the use of arbitrary untyped strings as keys.

The name Attributes is generic. If the type is retained, it should be renamed to ClientInfoAttributes.

I would address the issues above by using map[string]interface{} as the type for the attributes argument.

Treat that has feedback from an interested party. I am not the maintainer.

[justinmk]

attributes argument to nvim_set_client_info values are strings, but I don't see a corresponding restriction in the Neovim code. Should the type interface{} be used for the value?

Good question. Restriction doesn't exist technically, but I think Nvim maybe should restrict that to strings only, attributes was intended to be informational, not data storage. I will update the documentation and create an issue to add some validation.

[zchee]

@garyburd @justinmk Fixed SetClientInfo args type and some pointed out. here is description:

1. I'd left ClientType type and related constant variables. The Type field on struct Client must be constant variables defined by the ClientType type. But ClientType underlying types is a string type. Yes, plugin developers can pass untyped strings as keys, such as ClientType("foobar"). However, the Neovim core source seems to restrict to only string type for field value. So let's guaranteed it at the developer's responsibility.

2. Changed ClientInfoAttributes type to map[string]string. Fixed defined AttributesType map key to string type. Also, as @justinmk said, I thought the same thing. As a result of reading Neovim C sources, and tried to some debugging, map values seems also need a string type.

   Nvim maybe should restrict that to strings only, ...

3. I'd left AttributeKey{Website,License,Logo} even though I changed the ClientInfoAttributes type to map[string]string. Because in the :help nvim_set_client_info document:

   Clients might define their own keys, but the following are suggested: "website" ... "license" ... "logo" ...

   It is suggested, So I made it a just constant variable as a hint to developers. If there are plans to add many suggested keys in the future, and expected go-client maintenance costs of are concerned, I'll delete those. /cc @justinmk

4. Related testcase.

   Would it make sense to add a couple small tests for "sanity checking"? I think that not many tests are needed, but at least nvim_buf_attach was a bit tricky for other clients

   But some functionality such as nvim_buf_attach is worth testing.

   I was a little tired (means, "I'm tired a little") of the API implementation... :P As you said, yeah, nvim_buf_attach API is a bit tricky. But that API already working in my local environment and seems to have no bugs. I promise to send add test cases PR ASAP. Of course, before supporting other new APIs. So, is it possible to merge this PR first? (if @garyburd accept current implements.)

[justinmk]

However, the Neovim core source seems to restrict to only string type for map keys. So let's guaranteed it at the developer's responsibility.

Yes. The Nvim API is intentionally "loose" with respect to keys, for forward-compatibility.

It is suggested, So I made it a just constant variable as a hint to developers.

That seems ok.

I promise to send add test cases PR ASAP. Of course, before supporting other new APIs. So, is it possible to merge this PR first? (if @garyburd accept current implements.)

No problem.

[zchee]

@justinmk Thanks for quickly :)

@garyburd I have just a question. derail a little. You said

Pointers to structs should be used for "large" structs. The definition of "large" is a matter of judgement. The Version type in this PR is what I would call large.

I want to know this threshold for my future knowledge. What size do you judge it?

The Version struct types size is 56. all field is no pointer. 8(int) + 8(int) + 8(int) + 16(string) + 16(string) = 56 ref: golang-sizeof.tips

[zchee]

@garyburd Ah, forgot.

Treat that has feedback from an interested party. I am not the maintainer.

But but, I learned a lot of your code reviews. I know your busy, Thank you for polite reviews despite you busy.

[zchee]

@justinmk /cc @garyburd Let's merge it?

[zchee]

@justinmk Sorry noisy, just ping.

[zchee]

fixed bits.

[justinmk]

CI failure?

[zchee]

@justinmk Oh wow, I'll dig and fix it ASAP.

[zchee]

I understand, needs gofmt (like clang-format in C) to apitool.go. will do.

[zchee]

@justinmk PTAL.

[justinmk]

Would be best if @dzhou121, @akiyosi and any other go-client users gave feedback. But this has taken long enough, so we should just merge it.

[zchee]

@justinmk Rebased current master branch. Ready to merge!