Expose is_authenticated() helper
#519
Conversation
|
Hey, zbirenbaum rarely comes around these days but I do like the idea. :) It's been commented a few times that the plugin is too verbose when it is not authenticated or internet is unavailable, ideally this would move the plugin towards a more silent direction. Note that I am not requesting that you go ahead and move all notifications to :checkhealth (unless you are interested in undertaking a bigger task), just the 'not logged in' notifications are fine so that I may eventually move the rest in a similar fashion :) |
|
@AntoineGS that sounds like a good idea to me, I can look into making that change. That's a good point on moving the silenced pings to :checkhealth too, I'll see what I can do 👍 And thanks for the quick feedback! |
| local error = vim.health.error or vim.health.report_error | ||
| local info = vim.health.info or vim.health.report_info | ||
|
|
||
| function M.check() |
There was a problem hiding this comment.
Here's a screenshot of what this current healthcheck setup looks like:
lua/copilot/auth/init.lua
Outdated
| end | ||
| end | ||
|
|
||
| function M.is_authenticated() |
There was a problem hiding this comment.
@AntoineGS so I moved this over to use the lsp check_status endpoint. since it's async I threw some caching in here that tries to get the initial response from the lsp back ASAP, but I'm not super familiar with lsp communication setups like this so let me know if there's something better I can do here!
|
I like it! |
Hmm, okay I'll have to dig into those and see what's going on. I'm probably missing some test setup around the expectations for the async lsp response |
it looks like the suggestions tests are failing because the new is_authenticated check on schedule is blocking initial suggestions, I'll have to tweak my approach a bit to preserve existing speedy behavior for authenticated users
tests/child_helper.lua
Outdated
| @@ -105,6 +105,14 @@ function M.new_child_neovim(test_name) | |||
| ]]) | |||
| end | |||
|
|
|||
| function child.wait_for_lsp_authentication() | |||
There was a problem hiding this comment.
@AntoineGS I was able to get the rest of the tests passing with this new helper that waits until is_authenticated() is true
|
Tests look good, I will run it for a bit and merge if it looks good! |
lua/copilot/suggestion/init.lua
Outdated
| @@ -484,7 +485,7 @@ local function advance(count, ctx) | |||
| end | |||
|
|
|||
| local function schedule(ctx) | |||
| if not is_enabled() or not c.initialized then | |||
| if not is_enabled() or not c.initialized or not auth.is_authenticated() then | |||
There was a problem hiding this comment.
Reviewing the new flow and taking more time to understand why the tests were failing and how they were resolved, I can see that it is possible for Copilot to be ready and logged in but still not yield a suggestion, until the authentication is confirmed.
This will delay the first suggestion on a daily use -- slightly, but it may not be necessary.
If the first call to is_authenticated were to be synchronous (you could detect this by an empty cache timestamp) I believe it would solve this.
This would mean that the new changes to child_helper would not be needed, and removing it would be a way to test that the change works.
…tion tests" This reverts commit 1a31f1b.
|
I have reverted the last commit you had made adding the waits in the tests, and I think I found a good way to handle this. Let me know what you think! |
|
@AntoineGS I think your changes look great, I was thinking at the time that it'd be nice if there were a way to avoid needing to retry directly after the first auth attempt and your callback approach addresses that perfectly. I went through it again this morning and everything looks good on my end! |
|
hey @AntoineGS thanks for merging my PR! hopefully this PR isn't the root cause, but I'm noticing after updating my plugins that copilot is having some weird behavior. I'm seeing the I think there might need to be another call to copilot.lua/plugin/copilot.lua Line 52 in 8c4b70d |
|
Yeah someone commented on the commit so I reverted it right away, I am looking into it now. |
|
Hopefully the version I just pushed is fine! |
Hi @zbirenbaum, great plugin you've got here! I'm wondering if you'd be interested in merging something like this to solve a problem I'm running into.
I'm currently working on a neovim distro for my company that has a variety of users, and some don't want to auth into copilot for various reasons. These users would prefer that the plugin be disabled by default, because without authenticating, the plugin produces some noisy errors. I don't want to disable the plugin by default though, since the majority of users do and expect it to work automatically with our config without any manual tweaks on their part.
So my idea was to set up some more graceful default behavior for when a user is not authenticated, since ideally the plugin should be enabled and available to auth for anyone that wants to use it, but not noisily complain for anyone that opts not to use it. To do this, I threw in this
is_authenticated()function that checks for tokens or the creds file, and added an optional argument toget_creds()to avoid pinging notifications when this is hit. I then added this to the guard at the top of the suggestion schedule function so that suggestions don't get scheduled whenis_authenticated()is false.I'm not sure if this is the best approach to solve the problem I described, but this seems to be working so far