[RDY] represent api type String as msgpack type STR. #3431
Conversation
|
@bfredl - awesome. Will test asap... |
|
@bfredl - first thing I've found, MSGPACK requests from neovim to a client still appear to be encoding the method name as |
|
@myitcv Thanks, I tried to fix, please try again. I will look into adding tests... There was one test on api metadata that depended on STR/BIN encoding, but none for request/reply. |
|
Also noticed ShaDa uses |
|
Hmm looks like MessagePack.lua leaves STR and BIN indistinguishable, but we should be able to test it from python3. |
For neovim/neovim#3431 - neovim-qt did not accept the msgpack STR type (used BIN instead): - Neovim API signatures with type String - Msgpack request method names Now both types are treated the exact same way by the decode functions. This does not change the current API, we still treat strings as binary arrays (QByteArray), and we are still sending outgoing strings with type BIN.
|
Need to patch a couple of things, but otherwise it looks good on my end. I am still sending messages as BIN though (but since Neovim allows both it should be fine). |
|
Thinks look fine from JS world. I'll post back if I encounter any issues, though. |
For neovim/neovim#3431 - neovim-qt did not accept the msgpack STR type (used BIN instead): - Neovim API signatures with type String - Msgpack request method names Now both types are treated the exact same way by the decode functions. This does not change the current API, we still treat strings as binary arrays (QByteArray), and we are still sending outgoing strings with type BIN.
|
@ZyX-I Can you confirm the change to |
|
I am all for this. The changes to fix the code generation were trivial and it made me refactor the api parser and reduce it's complexity. 👍 |
|
@justinmk No, it will. But it is not much of a problem to fix. |
|
@bfredl Can you rebase and mark RDY? |
Should protect against neovim#3431
bfredl
changed the title
|
marked as RDY. |
Should protect against neovim#3431
|
FYI as I've reported in the @bfredl - thanks again, just tested these changes against |
Should protect against neovim#3431
8 participants
This changes the msgpack representations of strings in nvim from BIN to STR, as disscussed in #1250 (Only affects strings sent from nvim to clients, nvim already accepts both BIN and STR from clients).
Note that only
encoding=utf-8is fully supported, no conversion is done for other&encodings. This is already case when recieving utf-8 STR in requests, where no conversion is done for other encodings. As decided in #1250 remote plugins/GUIs are free to always assumeencoding=utf-8.I have checked python-client (both py2 and py3) and lua-client still works without changes, to the extent that tests still pass (except for unrelated failure in python also on master), but it would be good to check this in more clients before merging this. A client could be made compatible with nvim both before and after this change (by doing the right thing both when BIN and STR is recieved), so they can be updated if needed before this is merged.
ping @myitcv @nhynes @equalsraf @saep @rogual and anyone else implementing an API client/GUI.