-
Notifications
You must be signed in to change notification settings - Fork 32
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
base: master
Are you sure you want to change the base?
Conversation
5a9fd64
to
4d7c16c
Compare
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 |
This ensures that behavior between a newly opened nvim and one in which tabnine-chat has been opened and then closed are identical
guys what's the status here? |
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. |
@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() |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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
513ca36
to
c132306
Compare
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?