-
Notifications
You must be signed in to change notification settings - Fork 37.5k
Telescope config in plugin #1640
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
base: master
Are you sure you want to change the base?
Conversation
I think moving telescope keymaps out of lspconfig and into telescope's config is a good idea, but I still disagree with providing alternative picker in "kickstart.plugins.snacks" or anywhere else in the code. That just opens the door for a lot of other plugins to be included (alternative file browser plugins, alternative autopairs plugins etc.) which is completely unneeded. Kickstart shouldn't become a catalogue of plugins, it should stay small and reasonably usable. Every new nvim user will find out on his own soon after starting to use nvim that there's a ton of other plugins which he can use instead of the default ones chosen by kickstart if he's at least a bit interested in how nvim and the community around it works. If he's not interested, he should use LazyVim or VSCode or whatever. |
I mean it's not in the code. It's a comment. It's there to encourage people to make it their own. |
No, it encourages them to use snacks because it mentions snacks. If you want to encourage people to make the config their own, you shouldn't mention specific plugins, just add some general comment like "There are many other nvim plugins which add additional functionality to nvim or alternatives to plugins already included in kickstart". Everybody can type into google "nvim telescope alternative" and find out about snacks or fzf or any other alternative this way and kickstart's code and comments stay much cleaner this way and people don't have to fight about which plugin should be mentioned in the comments and which not, because all of them can't be mentioned, so who'll decide which will be? It's better to not mention any other plugins except those that are included in kickstart. There's already enough fighting about the default plugins included in kickstart, no need for even more fighting about plugins included in comments. |
yeah theres a bunch of plugins in there at the moment: https://github.com/nvim-lua/kickstart.nvim/blob/master/init.lua#L976 so if you want them removed, make a pr yourself, we can discuss that there. Until then im gonna leave this here, and maybe a maintainer has some thoughts on that and can chime in. |
But those aren't alternatives to a plugin that's already included in kickstart but plugins adding completely new features. |
-- This runs on LSP attach per buffer (see main LSP attach function in 'neovim/nvim-lspconfig' config for more info, | ||
-- it is better explained there). This is a little bit redundant, but we can switch off telescope for an optional | ||
-- picker like snacks more easily when the keymaps are defined in the plugin itself. | ||
-- It sets up buffer-local keymaps, autocommands, and other LSP-related settings | ||
-- whenever an LSP client attaches to a buffer. |
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.
As with my other comment above, I think it should be removed if it's redundant and the configs are intuitive (allows for easy plugin swapping). And the reasoning "when the keymaps are defined in the plugin itself" is more irrelevant to the user as it is more about a PR description.
Line 452-453 can be shortened to describe 455, but it should just be removed as well as this is redundant to what is noted anyway in parentheses.
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 dont know know about you, but when i see the telescope plugin with those autocommands, im kinda confused when there's no clarification there. So im gonna leave the reference in there. This is only for people new to the config, one can always remove stuff if they make it their own.
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.
LGTM :)
I previously added these changes to another pr of mine, but after some thought, im pulling those changes apart, so we can decide on replacing the picker, more infos here: #1632
this pr just moves some telescope specific config from the lspconfig to the telescope plugin config declaration, just to entangle those two.