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

plugins/fzf-lua: init + test #793

Merged
merged 1 commit into from
Feb 14, 2024
Merged

Conversation

budimanjojo
Copy link
Contributor

No description provided.

@budimanjojo
Copy link
Contributor Author

I don't know why the test fails, I tested locally without my addition and it fails too.

@budimanjojo budimanjojo marked this pull request as draft December 8, 2023 04:34
@budimanjojo budimanjojo force-pushed the fzf branch 2 times, most recently from 5cc941d to f77c4ce Compare December 8, 2023 04:44
@budimanjojo budimanjojo marked this pull request as ready for review December 8, 2023 04:49
@budimanjojo
Copy link
Contributor Author

Okay I managed to fix the failed check issue 🥳

plugins/utils/fzf-lua.nix Outdated Show resolved Hide resolved
@budimanjojo
Copy link
Contributor Author

@GaetanLepage Done!

@budimanjojo
Copy link
Contributor Author

@GaetanLepage is there any other issue that I need to fix?

@GaetanLepage
Copy link
Member

Hey, sorry for taking so long.
Everything looks nice for now. However, I would encourage you to wrap the plugin options like we do for the other plugins.

The simpler way would be to limit the scope to the "global options" that would end up in the setupOptions attrs.

If you want to go a bit further, you could create a keymaps option that allows the user to create mappings easily (like we do for telescope or harpoon for instance).
You could even add options for the keymaps to be configurable.

I suggest to limit the scope of this PR to the default options though.

@budimanjojo
Copy link
Contributor Author

Thanks! I'll try to add keymaps and keymapsSilent option for now. I don't think I understand the "wrap the plugin options like we do for the other plugins" part. Do u mean I need to add more options instead of making users to use extraOptions for everything else?

@GaetanLepage
Copy link
Member

Do u mean I need to add more options instead of making users to use extraOptions for everything else?

Yes exactly. It is a bit of a pain to do, but it is one of the added value of nixvim ;)

@budimanjojo
Copy link
Contributor Author

budimanjojo commented Dec 14, 2023

@GaetanLepage I added keymaps options. I don't know if my nix knowledge is good enough to create all those setup options without making the code looks messy. Maybe it's just me but I think people should just be good with changing the provided profile (or theme) and override the provided profile options per keymap like I provided in the example.

Maybe you can accept this PR and maybe if somebody else want to improve this plugin then he/she/they can make another PR?

@budimanjojo
Copy link
Contributor Author

budimanjojo commented Dec 14, 2023

I also agree that having the setup options added in can be some kind of type checking at build time in case the user provided a wrong options it will fail to build resulting in the user must fix that up to continue building instead of having a broken neovim config. But I'm just not good enough to make it robust. I think the better way to go is to have all the available options in a variable that can also be used for the actionOpts in the keymaps in my last commit.

But, there are a lot of options that are action name specific, i.e cwd is only available in files action and this makes my brain hurts just by thinking about it 😅

Copy link
Member

@GaetanLepage GaetanLepage left a comment

Choose a reason for hiding this comment

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

I think that what you have done until now is good.

I would still want to package the options.
If you allow me to do so, I can complete this work by force-pushing to your branch.
Does that sound good to you ?

plugins/utils/fzf-lua.nix Outdated Show resolved Hide resolved
plugins/utils/fzf-lua.nix Outdated Show resolved Hide resolved
plugins/utils/fzf-lua.nix Outdated Show resolved Hide resolved
@budimanjojo
Copy link
Contributor Author

@GaetanLepage I have commited the requested changes. Yes feel free to push to the branch! Thank you!

@GaetanLepage GaetanLepage force-pushed the fzf branch 2 times, most recently from 61d3066 to d021c6b Compare February 14, 2024 21:49
@GaetanLepage
Copy link
Member

This is finally ready ! Sorry @budimanjojo for taking that long...

@GaetanLepage GaetanLepage requested a review from traxys February 14, 2024 21:49
@GaetanLepage GaetanLepage force-pushed the fzf branch 2 times, most recently from 62dcaab to b038640 Compare February 14, 2024 22:25
@GaetanLepage GaetanLepage merged commit 5a744a7 into nix-community:main Feb 14, 2024
39 checks passed
@budimanjojo
Copy link
Contributor Author

Thank you!

@budimanjojo budimanjojo deleted the fzf branch February 15, 2024 04:29
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.

3 participants