-
Notifications
You must be signed in to change notification settings - Fork 41
wip(msgpack): handle Nil strings #184
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
base: main
Are you sure you want to change the base?
Conversation
for nvim minor verion 9, no commands are returned for nvim minor version 10, three are returned currently, for nightly builds, four are returend
neovim ships with default keybindings that may change per verison. the tests, prior to this change, included these default key mappings which make the tests fragile with each release. keeping a list of default commands should be outside the scope of this library. this change removes assertions against the entire set of keymaps returned by `nvim_get_keymap` and only verifies the desired keymap has been added and then removed.
using the colors described here rather than the color map https://neovim.io/doc/user/syntax.html#ctermbg
v0.11 changed the way that messages are broadcast from only to those who are subscribed, to all channels. this test, as it's written, doesn't make sense in that scenario. snippet from release notes: > vim.rpcnotify(0) and rpcnotify(0) broadcast to ALL channels. Previously > they would "multicast" only to subscribed channels (controlled by > nvim_subscribe()). Plugins and clients that want "multicast" behavior > must now maintain their own list of channels.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #184 +/- ##
=======================================
- Coverage 83.3% 83.1% -0.2%
=======================================
Files 14 14
Lines 3132 3134 +2
=======================================
- Hits 2610 2607 -3
- Misses 522 527 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
all of the listed go versions prior to this change are no longer supported. this bumps to more recent versions.
|
Is this still WIP? |
|
I forgot about this. after looking at this again with fresh eyes and a little bit more of a desire to figure it out (rather than just fix) I think I can say that the concern I described in the initial description is not an issue. for context, I was worried that dropping the colormap wasn't really fixing the failing test but just making it not visible. I now think that the test set up was incorrectly to begin with. This was the original error: --- FAIL: TestAPI/Highlight (0.00s)
--- FAIL: TestAPI/Highlight/Nvim (0.00s)
api_test.go:3865: got cterm foreground is 0 but want 16711680
--- FAIL: TestAPI/Highlight/Batch (0.00s)
api_test.go:3946: got cterm foreground is 0 but want 16711680
FAILthe nvim_set_hl api is being called with as for how it ended up as 0, I think nvim_set_hl overflows during the cast to an int16_t -- I didn't see any validation, but I might have just missed it. anyway, I think this is ready for a review if @zchee or @justinmk would like to take a look. if you don't agree with my analysis, I'm happy to make any changes or look at it some more. some of the tests cold probably get some more love. |
I made another change that I'm worried might not be the right direction:
Here
https://github.com/neovim/go-client/pull/184/files#diff-4686cdf50ccf547850a2e89191f11c343519caef6fd7718fd73b7a1480606bd0L3843
I moved away from using the color map based on https://neovim.io/doc/user/syntax.html#ctermbg. I'm not sure if the ctermbg / ctermfg are expected to work with the RGB colors that are in the color map, or if maybe the color map has been enhanced since v0.9? I'm not sure if I'm avoiding the issue with this fix.