Support neovim/neovim@a225374 api #36

zchee · 2018-08-05T10:47:21Z

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 · 2018-08-05T10:47:32Z

/cc: @garyburd

zchee · 2018-08-05T12:04:03Z

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

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

justinmk · 2018-08-05T12:10:48Z

@zchee yes

zchee · 2018-08-05T12:19:42Z

@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?

nvim/apidef.go

nvim/nvim.go

zchee · 2018-08-13T15:45:26Z

/ping @garyburd

ghost · 2018-08-14T04:07:55Z

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 · 2018-08-14T14:51:24Z

@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)

ghost · 2018-08-15T16:19:54Z

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 · 2018-08-28T17:20:40Z

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

zchee · 2018-08-28T18:32:46Z

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

zchee · 2018-08-31T05:45:00Z

/ping @garyburd @justinmk

zchee · 2018-09-07T17:49:34Z

sorry, re-ping: @garyburd @justinmk

justinmk · 2018-09-07T18:10:21Z

@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.

ghost · 2018-09-13T17:25:37Z

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 · 2018-09-26T08:44:07Z

@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.
neovim/neovim@45f53b3

ghost · 2018-09-26T23:17:05Z

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 · 2018-09-27T04:40:00Z

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

zchee · 2018-12-01T03:10:22Z

@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 · 2019-01-08T10:37:28Z

@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
#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 · 2019-01-08T11:24:09Z

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 · 2019-01-08T14:14:51Z

@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/[email protected]
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.

ghost · 2019-01-11T17:30:51Z

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:

Declare and use a type for commands. This is the best option.
Omit the method from the PR. This is the least amount of work.
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.

ghost · 2019-01-11T18:38:48Z

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 · 2019-01-14T15:43:13Z

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

zchee · 2019-03-04T18:40:48Z

@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?

ref: golang/go@9a0a150

zchee · 2019-04-23T12:52:28Z

@justinmk Rebased current master branch. Ready to merge!

justinmk · 2019-04-24T08:27:39Z

nvim/types.go

+
+	// Blocking is true if Nvim is waiting for input.
+	Blocking bool `msgpack:"blocking"`
+}


why make a new type for this? This seems over-specific. Can't it just be a map? I guess go does not allow heterogeneous maps so it would be map[string]interface{}.

If having types for every little thing is idiomatic in go, then I guess this is ok. But it means that the client will not be forward-compatible if Nvim API adds a new field to the map. :help api-contract and :help dev-api-client explain that clients should be prepared for new, optional fields to appear in any map or list.

#36 (comment) leads me to believe this will be forward-compatible. If so, OK.

@justinmk I didn't add a new type.
Just move some types to types.go such as

go-client/nvim/nvim.go

Lines 645 to 691 in fba3ac8

type Mode struct {

// Mode is the current mode.

Mode string `msgpack:"mode"`

// Blocking is true if Nvim is waiting for input.

Blocking bool `msgpack:"blocking"`

}

type HLAttrs struct {

Bold bool `msgpack:"bold,omitempty"`

Underline bool `msgpack:"underline,omitempty"`

Undercurl bool `msgpack:"undercurl,omitempty"`

Italic bool `msgpack:"italic,omitempty"`

Reverse bool `msgpack:"reverse,omitempty"`

Foreground int `msgpack:"foreground,omitempty" empty:"-1"`

Background int `msgpack:"background,omitempty" empty:"-1"`

Special int `msgpack:"special,omitempty" empty:"-1"`

}

type Mapping struct {

// LHS is the {lhs} of the mapping.

LHS string `msgpack:"lhs"`

// RHS is the {hrs} of the mapping as typed.

RHS string `msgpack:"rhs"`

// Silent is 1 for a |:map-silent| mapping, else 0.

Silent int `msgpack:"silent"`

// Noremap is 1 if the {rhs} of the mapping is not remappable.

NoRemap int `msgpack:"noremap"`

// Expr is 1 for an expression mapping.

Expr int `msgpack:"expr"`

// Buffer for a local mapping.

Buffer int `msgpack:"buffer"`

// SID is the script local ID, used for <sid> mappings.

SID int `msgpack:"sid"`

// Nowait is 1 if map does not wait for other, longer mappings.

NoWait int `msgpack:"nowait"`

// Mode specifies modes for which the mapping is defined.

Mode string `msgpack:"string"`

}

justinmk · 2019-04-24T08:28:46Z

nvim/types.go

+	Special    int  `msgpack:"special,omitempty" empty:"-1"`
+}
+
+// Mapping represents a nvim mapping options.


That comment doesn't really add any information. But at least "a nvim" should be just "Nvim" (no "a").

@justinmk I see will fix.

justinmk · 2019-04-24T08:30:55Z

nvim/types.go

+	// LHS is the {lhs} of the mapping.
+	LHS string `msgpack:"lhs"`
+
+	// RHS is the {hrs} of the mapping as typed.


Suggested change

// RHS is the {hrs} of the mapping as typed.

// RHS is the {rhs} of the mapping as typed.

or just:

Suggested change

// RHS is the {hrs} of the mapping as typed.

// RHS of the mapping.

BTW, why does every comment start with "Foo is ..." ? It's redundant.

It is coding standard in Go. Golang linters demand that every doc string on function, struct or other declaration starts with its name. As I know it is Rob Pike idea.

It could be seemed redundant but in fact it simplifies understanding since doc strings has predefined structure.

That comment doesn't really add any information. But at least "a nvim" should be just "Nvim" (no "a").

Nevertheless, sometimes such 'comment style' results in useless and meaningless descriptions like above.

@daskol Thanks for description :)

justinmk · 2019-04-24T08:43:34Z

nvim/types.go

+// For a host, this does not include plugin methods which will be discovered later.
+// The key should be the method name, the values are dicts with the following (optional) keys. See below.
+//
+// Further keys might be added in later versions of nvim and unknown keys are thus ignored.


Ah, ok. Good.

justinmk · 2019-04-24T08:46:08Z

nvim/apiimp.go

@@ -123,6 +123,50 @@ func (b *Batch) BufferLines(buffer Buffer, start int, end int, strict bool, resu
 	b.call("nvim_buf_get_lines", result, buffer, start, end, strict)
 }

+// AttachBuffer activate updates from this buffer to the current channel.


I recently updated the Nvim docs for the attach methods. https://neovim.io/doc/user/api.html#nvim_buf_attach()
Could help with clarity/wording of these attach/detach docstrings.

Suggested change

// AttachBuffer activate updates from this buffer to the current channel.

// Activates buffer-update events on the channel.

Also will fix.

ghost reviewed Aug 5, 2018

View reviewed changes

nvim/apidef.go Outdated Show resolved Hide resolved

nvim/apidef.go Outdated Show resolved Hide resolved

nvim/apidef.go Outdated Show resolved Hide resolved

nvim/nvim.go Outdated Show resolved Hide resolved

nvim/nvim.go Outdated Show resolved Hide resolved

zchee self-assigned this Dec 1, 2018

zchee added the area/nvim label Dec 1, 2018

zchee mentioned this pull request Dec 1, 2018

Support neovim/neovim@0ce8800 api #40

Closed

This comment has been minimized.

Sign in to view

zchee added 21 commits April 23, 2019 21:50

nvim: implements Client struct

33382bc

nvim: add godoc comment to Channel and Process struct

ef3c0b7

nvim: split additional struct types to types.go

4ae47cc

nvim/apitool: add some C type to nvimTypes map

c13a168

nvim: add Version,ClientType,Methods,Attributes and Client for Channel

ea2d236

nvim: go generate

606b164

nvim: add Command type and fix {Buffer}Commands return map type

2c7dd6c

nvim: go generate

d1b7993

nvim: change Version type to struct instead of map

44b42c1

nvim: map[string]*Command type to Dictionary

060a4c2

nvim: fix SetClientInfo arguments type

12eff30

nvim: re-implements several types for SetClientInfo function

1e4b674

nvim: go generate

272d59a

nvim: fix Version.Major msgpack struct tag

92cba7f

nvim: increase LICENSE Year

8525170

nvim: rename some Client related type name to add Client prefix

c23f16a

nvim: go generate

12a1e2e

nvim: add some godoc comment and format

abf9cd9

nvim: fix spell "Deprecated: Use etc" consistently

da61368

ref: golang/go@9a0a150

nvim: go generate

c8e9cd2

nvim: gofmt

06d42d7

justinmk reviewed Apr 24, 2019

View reviewed changes

justinmk merged commit ada0455 into neovim:master Apr 24, 2019

zchee deleted the api/a225374 branch May 2, 2019 06:29

zchee mentioned this pull request May 9, 2019

Support call nvim_set_client_info when connecting to set default channel info #48

Open

	type Mode struct {
	// Mode is the current mode.
	Mode string `msgpack:"mode"`

	// Blocking is true if Nvim is waiting for input.
	Blocking bool `msgpack:"blocking"`
	}

	type HLAttrs struct {
	Bold bool `msgpack:"bold,omitempty"`
	Underline bool `msgpack:"underline,omitempty"`
	Undercurl bool `msgpack:"undercurl,omitempty"`
	Italic bool `msgpack:"italic,omitempty"`
	Reverse bool `msgpack:"reverse,omitempty"`
	Foreground int `msgpack:"foreground,omitempty" empty:"-1"`
	Background int `msgpack:"background,omitempty" empty:"-1"`
	Special int `msgpack:"special,omitempty" empty:"-1"`
	}

	type Mapping struct {
	// LHS is the {lhs} of the mapping.
	LHS string `msgpack:"lhs"`

	// RHS is the {hrs} of the mapping as typed.
	RHS string `msgpack:"rhs"`

	// Silent is 1 for a \|:map-silent\| mapping, else 0.
	Silent int `msgpack:"silent"`

	// Noremap is 1 if the {rhs} of the mapping is not remappable.
	NoRemap int `msgpack:"noremap"`

	// Expr is 1 for an expression mapping.
	Expr int `msgpack:"expr"`

	// Buffer for a local mapping.
	Buffer int `msgpack:"buffer"`

	// SID is the script local ID, used for <sid> mappings.
	SID int `msgpack:"sid"`

	// Nowait is 1 if map does not wait for other, longer mappings.
	NoWait int `msgpack:"nowait"`

	// Mode specifies modes for which the mapping is defined.
	Mode string `msgpack:"string"`
	}

	// RHS is the {hrs} of the mapping as typed.
	// RHS is the {rhs} of the mapping as typed.

	// RHS is the {hrs} of the mapping as typed.
	// RHS of the mapping.

	// AttachBuffer activate updates from this buffer to the current channel.
	// Activates buffer-update events on the channel.

Support neovim/neovim@a225374 api #36

Support neovim/neovim@a225374 api #36

Uh oh!

Conversation

zchee commented Aug 5, 2018

Uh oh!

zchee commented Aug 5, 2018

Uh oh!

zchee commented Aug 5, 2018

Uh oh!

justinmk commented Aug 5, 2018

Uh oh!

zchee commented Aug 5, 2018

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zchee commented Aug 13, 2018

Uh oh!

ghost commented Aug 14, 2018

Uh oh!

zchee commented Aug 14, 2018

Uh oh!

ghost commented Aug 15, 2018

Uh oh!

zchee commented Aug 28, 2018

Uh oh!

zchee commented Aug 28, 2018

Uh oh!

zchee commented Aug 31, 2018

Uh oh!

zchee commented Sep 7, 2018

Uh oh!

justinmk commented Sep 7, 2018

Uh oh!

ghost commented Sep 13, 2018

Uh oh!

zchee commented Sep 26, 2018

Uh oh!

ghost commented Sep 26, 2018

Uh oh!

zchee commented Sep 27, 2018

Uh oh!

zchee commented Dec 1, 2018

Uh oh!

zchee commented Jan 8, 2019

Uh oh!

justinmk commented Jan 8, 2019

Uh oh!

zchee commented Jan 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Jan 11, 2019

Uh oh!

ghost commented Jan 11, 2019

Uh oh!

zchee commented Jan 14, 2019

Uh oh!

zchee commented Mar 4, 2019

Uh oh!

This comment has been minimized.

zchee commented Apr 23, 2019

Uh oh!

justinmk Apr 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

zchee commented Jan 8, 2019 •

edited

Loading

justinmk Apr 24, 2019 •

edited

Loading

daskol Apr 24, 2019 •

edited

Loading

justinmk Apr 24, 2019 •

edited

Loading