Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
WIP Integrations API Proposal (+ whichkey and mapper integrations) #31
base: master
Are you sure you want to change the base?
WIP Integrations API Proposal (+ whichkey and mapper integrations) #31
Changes from 14 commits
8d104ad
faee545
7aef767
56823c5
f086d45
2365f44
295f582
58368d7
cfae439
8eecc9c
7e881ec
f524511
5de6099
2645f86
e6647e3
19427cf
d5ff3cc
56e3759
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Here the example uses
<leader>f
forTelescope find_files
, and in other spots (line 264 and original README) it uses<leader>ff
Suggestion: change it all to
<leader>ff
, just for consistency.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.
default_integration
could be pulled out into it's own file, for consistency, because the other integrations are in their own files anyways.Only reason to not would be if some user accidently nuked their integrations folder, having this in
nest.lua
would enable them to still run the barebones version of the plugin.If someone deletes integrations, they can re-clone the rep. So my vote is on extract for consistency.
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 agree with splitting it into its own file but one of the selling points in the README.md was that it was a single lua file. Interested what @LionC thinks. On the other hand given that the vim.keymap api is merged it could be worth having two default integrations, one for the new API (no need to manage a table of lua functions) and one for the old API. Not sure if there's any benefit to doing this however.
Also for your last point, I don't think users will ever be deleting the contents of the integrations folder, it's just supposed internal to nest.nvim. If a user want's to add their own custom integration they can just create an object with the same fields as the example integration and pass it using
nest.enable()
.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 completely missed that! This PR would also crush the 100 lines claim, though. Maybe 2 separate branches could serve different versions, one that's
minimal
and the otherintegrated
. Though that certainly raises up the ante for maintaining this project quite a bit.Oh wow! I had no idea they were doing this. It certainly would help pushing all the lifting onto the Nvim API itself.(also reduce lines of code, if that philosophy is still maintained).
And no, I don't think there would be a benefit maintaining for an older API. I mean, we could merge this PR as is and then modify it for new API, redirecting all who want the old API to the merge commit 🤷. Or if it is that troublesome, just have the old stuff be fallback. Adds bloat, but backwards compatibility.
OR (Ikr, too many ideas!) just split breaking stuff into different file with wrappers around the breaking stuff, and ask users to include whichever one fits their Nvim version.
Yeah, I know, I was just thinking of worst case scenarios. Just a couple of days ago packer kinda broke on me and would not delete the plugin folder when removing it, only it's contents. That led to it asking me repeatedly if I wanted to remove it, and not doing so, rinse and repeat. I got fed up, went to the plugins folder and did stuff that broke packer! So yeah.