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

feat: health check #1267

Merged
merged 8 commits into from
Jun 15, 2024
Merged

feat: health check #1267

merged 8 commits into from
Jun 15, 2024

Conversation

folke
Copy link
Contributor

@folke folke commented Jun 14, 2024

Health check that tests some basic things for a user to troubleshoot fzf-lua.

There's a bug in Neovim that prevents the healthcheck to work, when its in lua/fzf-lua/health.lua.
For now I've put it in lua/fzf/health.lua, but this is of course not ideal.
The issue can be fixed upstream, but then non-nightly Neovim installs would fail running the health checks...

You can test it with :checkhealth fzf_lua.

This is how it looks for me:

image

@ibhagwan
Copy link
Owner

Looks great @folke.

A few observations:

  • Given we decided we're accepting that some users reuse existing $FZF_DEFAULT_OPTS should we warn about this?
  • Fzf-lua doesn't require fd, sk, bat or delta, it can work with the default grep on Linux/Mac and requires rg only on windows and can list files with the default find or dir.exe on Windows.
  • It seems that health only supports OK or WARNING, in the above case does it make sense to warn about missing delta given that it's totally optional?

@folke
Copy link
Contributor Author

folke commented Jun 15, 2024

Added the additional checks per OS and removed the warning:

image

@folke
Copy link
Contributor Author

folke commented Jun 15, 2024

Requirements are now correctly checked for each OS

@folke
Copy link
Contributor Author

folke commented Jun 15, 2024

I was able to work-around that Neovim bug with health.
Instead of adding lua/fzf/health.lua, I've moved the heath check to autoload/health/fzf_lua.vim.
Which then calls lua/fzf-lua/_health.lua. Note that the file can't be named health.lua, or it will trigger errors.

You can use :checkhealth fzf_lua to just check for fzf-lua

@folke
Copy link
Contributor Author

folke commented Jun 15, 2024

@folke folke changed the title feat: health check WIP feat: health check Jun 15, 2024
@ibhagwan
Copy link
Owner

I was able to work-around that Neovim bug with health.
Instead of adding lua/fzf/health.lua, I've moved the heath check to autoload/health/fzf_lua.vim.
Which then calls lua/fzf-lua/_health.lua. Note that the file can't be named health.lua, or it will trigger errors.
You can use :checkhealth fzf_lua to just check for fzf-lua

That’s great @folke, tysm!

Added an extra check for stdpath.run.

Good idea, I’ll test this on both Mac and Windows.

@ibhagwan
Copy link
Owner

Added some more ideas in the code, lmk what you think or if you’d like me to add the proposed changes.

@folke
Copy link
Contributor Author

folke commented Jun 15, 2024

You probably forgot to push your changes? I don't see anything here, no review or code updates.
But for sure, change it to your liking :)

@ibhagwan
Copy link
Owner

You probably forgot to push your changes? I don't see anything here, no review or code updates. But for sure, change it to your liking :)

quite frankly, I’ve never done a proper “review” I just wanted to add comments with line context and it opened a review, I usually do a manual rebase, modify, and change commits on the command line, never figured how to propagate these back to an open PR lol

@folke
Copy link
Contributor Author

folke commented Jun 15, 2024

When you add comments in a review, at the end you need to click top right, to submit the review, so that the comments show up

@ibhagwan
Copy link
Owner

When you add comments in a review, at the end you need to click top right, to submit the review, so that the comments show up

Gonna try it for fun :)

lua/fzf-lua/_health.lua Outdated Show resolved Hide resolved
lua/fzf-lua/_health.lua Outdated Show resolved Hide resolved
lua/fzf-lua/_health.lua Outdated Show resolved Hide resolved
@folke
Copy link
Contributor Author

folke commented Jun 15, 2024

With the gh tool, you can actually make changes to a PR yourself.

First do gh pr checkout 123

Test, make changes and you can then do a simple git push to update the PR. Super useful. I do that all the time

@ibhagwan
Copy link
Owner

With the gh tool, you can actually make changes to a PR yourself.

First do gh pr checkout 123

Test, make changes and you can then do a simple git push to update the PR. Super useful. I do that all the time

Oh nice, I just stick to vanilla git with an alias to pull PRs locally but this is a good reason to use the tool

  fpr   = "!f(){ git fetch origin pull/${@}/head:pr_${@}; }; f"

I was wondering why people never seem to see my code comments lol

@ibhagwan
Copy link
Owner

Hi @folke, just saw your changes for the directory writeable test, I also added 357339d, might seem like an overkill but I've also seen a case where serverstart() failed with a different error (#1226) so maybe we should keep this test?

@folke
Copy link
Contributor Author

folke commented Jun 15, 2024

yep, makes sense!

@ibhagwan
Copy link
Owner

If you feel this is ready to be merged I can ship it :-)

@folke
Copy link
Contributor Author

folke commented Jun 15, 2024

Go for it and thank you! :)

@ibhagwan ibhagwan merged commit f21ffe4 into ibhagwan:main Jun 15, 2024
@ibhagwan
Copy link
Owner

Tysm @folke, this has been a great help and for sure will prove very valuable in future troubleshooting .

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.

2 participants