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

Handle errors in finding binary path #82

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

aarondill
Copy link
Contributor

This avoids constant nil errors (fixes #77), and quits early if the binary can't be found.
Notifications can easily be added in future commits if so desired to warn the user about what is wrong.

to @amirbilu and @vendion, should I add these notifications, or is a silent fail preferable? or, should we write to config.log_file_path if it's defined?

@vendion
Copy link

vendion commented Jun 19, 2023

I would vote for either silent fail or logging, as a notification would be annoying as well (although shouldn't negatively affect Neovim's usability). I don't need to be constantly reminded that Tabnine doesn't support all the platforms that I use, and would more than likely make me consider canceling my pro subscription.

@aarondill
Copy link
Contributor Author

I would vote for either silent fail or logging, as a notification would be annoying as well (although shouldn't negatively affect Neovim's usability). I don't need to be constantly reminded that Tabnine doesn't support all the platforms that I use, and would more than likely make me consider canceling my pro subscription.

I could understand not wanting a notification on unsupported hardware (though that could be solved in lazy.nvim btw), but should we notify if the binary is not there? that means either the build script failed and needs to be rerun, or else the user (accidentally?) deleted the binary folder w/o rerunning the script.

aarondill referenced this pull request in aarondill/tabnine-nvim Jul 7, 2023
This ensures that behavior between a newly opened nvim and one in which tabnine-chat has been opened and then closed are identical
@amirbilu
Copy link
Contributor

guys what's the status here?

@aarondill
Copy link
Contributor Author

We were waiting on your input on the notifications, I believe, but I just added them for missing files, if the arch is in our list of supported ones. So this should now be ready to merge.

@aarondill
Copy link
Contributor Author

@amirbilu any update on getting a review here?

@@ -36,6 +38,11 @@ local function binary_name()
end

local function binary_path()
local machine = arch_and_platform()
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you say about rewriting the build script to lua? and have it run on initialization

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could do it, but we would end up forking to a shell for curl, unzip and chmod (and their windows equivalents) because there’s no way to do those in lua without external libraries. If thats what you want, I’ll start another PR and do that, but that seems out of scope of this PR

Copy link
Contributor

Choose a reason for hiding this comment

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

I get it. I believe it would make this a bit easier to manage... I didn't do it to begin with since it requires extra effort which I didn't have at the time.

As for this PR. I would expect binary_path() to throw an error in case it fails to find the binary for any case. The current implementation is too scattered in my opinion

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can make binary_path to throw an error, but I would just end up calling pcall on it so we can give a nice error. It makes sense that nil represents a value that couldn't be found, which we can then interpret to give the user a warning.

Additionally, binary_path throwing an error was the original issue of #77, in which his hardware isn't supported, so it never finds the binary_path and repeatedly throws errors.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, i was aiming to use pcall & yield the error in one place. does not make sense to you?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I never saw this reply.

It does not make sense to me to use pcall higher in the call stack when we can use nil and check for it instead.

In my past experience using pcall to detect errors is unwieldy. Error() is kinda like JavaScript's throw, in where you can technically pass any data type, but most people use X (strings and Errors, respectively). This makes it hard to properly handle errors, and even if strings are properly passed and determined, they are hard to parse due to the possibility that any operation or function call errored and it climbed the call stack. Additionally the inclusion of a call stack in the error string means that we may not even be able to reliably parse the error without a complicated pattern, which would be very prone to typos and bugs.

I would much prefer to keep this limited to the one area, where we can easily check for nil (and an lsp can warn you if you forget).

Furthermore, throwing an error is exactly the problem that caused this PR. @vendion is using unsupported hardware and since this function is called (and errors) frequently, he receives constant error messages (on cursorhold). This pr intends to fix that by detecting misconfigurations and unsupported hardware and warning once for configuration, and being silent for unsupported hardware (per @vendion's request)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@amirbilu do you have feedback on this response?
If you think it's a better approach, I can change to using pcall, but this will likely make debugging and detecting issues harder in the future. I personally think it's a better idea to return an explicit not-supported value (nil in this case), and handle it directly at the call site.

This avoids constant nil errors (fixes codota#77), and quits early if the binary can't be found.
Notifications can easily be added in future commits if so desired to warn the user about what is wrong.
This ensures we don't write to an empty pipe.
…rly.

This avoids notifying when the machine is unsupported.
This is prefered behavior by *at least* @vendion
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.

Constant "attempt to concatenate a nil value error"
3 participants