Skip to content
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

Improve callAtomic error logging #1110

Merged
merged 7 commits into from
Oct 30, 2023

Conversation

pranavrajpal
Copy link
Contributor

I spent a lot of time trying to figure out why this extension and Neovim's view of the current file always seemed to ignore any changes made in insert mode, only to realize that it was caused by my installed Neovim being too old to have nvim_buf_set_text defined (which was necessary after #992).

This PR makes a few changes to make errors like that easier to discover by:

  • Correcting the minimum Neovim version check to check for the actual minimum version of 0.8.0
  • Changing callAtomic's error handling to actually report all errors to the user (I suspect the old implementation wasn't actually reporting any errors since it seems like it was looking for errors in the wrong place)
  • Running CI on the oldest supported Neovim version to avoid accidentally depending on newer features

The extra error reporting in callAtomic seems to be surfacing some old errors (so far I've found errors in both nvim_win_close and nvim_buf_set_name) and I'm not sure whether we should silence those for now or leave them on until they're fixed. For now, I've silenced nvim_win_close errors but I'm not sure whether that's the right decision.

Neovim seems to be completely ignoring any changes made in insert mode,
which is causing neovim's buffer to get out of sync with the version of
the file in VSCode.
The issue with insert mode changes getting ignored is caused by
nvim_buf_set_text not being recognized by an old Neovim version
installed on my computer (somewhere around 0.5.0), so make this more
obvious by reporting an error if there the Neovim version than the
current minimum required version (which is 0.8.0 according to the
readme).
The previous bug caused by nvim_buf_set_text not existing on older
Neovim could have been noticed if you saw the error messages returned by
Neovim, which included an "Invalid method: nvim_buf_set_text" error.

Make bugs like this easier to find in the future by correctly logging
those Neovim-returned errors.

Also silence errors due to nvim_win_close calls since there seem to be
several errors mentioning an invalid window ID being passed to it and
those errors are currently probably more annoying than helpful.
Make bugs involving old Neovim versions more apparent by running CI on
the oldest supported version, so that we don't accidentally end up
relying on features introduced in later versions.
@pranavrajpal
Copy link
Contributor Author

I don't really understand why the CI is failing. The "extmark display overlay" test is causing the error "nvim_call_function: Vim:E5555: API call: col value outside range", but I can't reproduce that error on my computer.

@theol0403
Copy link
Member

Thank you for the PR! The error logging changes need a bit more attention (to make sure it won't cause problems, and maybe fix the underlying problems), so I have gone ahead and cherry-picked the simpler changes in #1115. Feel free to keep working on this PR!

I'm not sure if the failing test is your fault.

@theol0403 theol0403 changed the title Make errors for using outdated Neovim more apparent Improve callAtomic error logging Dec 30, 2022
@theol0403 theol0403 marked this pull request as draft July 5, 2023 23:17
@xiyaowong xiyaowong marked this pull request as ready for review October 30, 2023 12:59
Copy link
Member

@theol0403 theol0403 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See if the window close error can be avoided

@theol0403 theol0403 merged commit 91c0169 into vscode-neovim:master Oct 30, 2023
7 of 8 checks passed
@xiyaowong
Copy link
Collaborator

In most cases, we don't care about the results of nvim_win_close, just keep the log for the maintainers to see.

@xiyaowong
Copy link
Collaborator

I just discovered that there are so many descriptions. In fact, these are useless and should be deleted.
image

@theol0403
Copy link
Member

It's true that most of them aren't relevant, but I don't think it really matters

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants