Refactor fs.fnl to use nvim 0.8 builtins #630
Conversation
saccarosium
force-pushed
the
cleanup-old-batteries
branch
from
9a173e0 to
d6dcdbc
Compare
saccarosium
force-pushed
the
cleanup-old-batteries
branch
from
d6dcdbc to
b785a53
Compare
| (local config (autoload :conjure.config)) | ||
|
|
||
| (local path-sep (if (= jit.os :Windows) "\\" "/")) |
There was a problem hiding this comment.
This is implemented in nfnl.fs/path-sep as a function. I'll replace this though, no biggie!
| (fn env [k] | ||
| (let [v (nvim.fn.getenv k)] | ||
| (let [v (vim.fn.getenv k)] |
There was a problem hiding this comment.
Good call, I like these getting replaced with the built in API now.
| (text.ends-with file-path (.. mod-path ".fnl")) | ||
| (text.ends-with file-path (.. mod-path "/init.fnl"))) | ||
| (vim.endswith file-path (.. mod-path ".fnl")) | ||
| (vim.endswith file-path (.. mod-path "/init.fnl"))) |
| require("conjure.main").main() | ||
| else | ||
| vim.notify_once("Conjure requires Neovim > v0.8", vim.log.levels.ERROR) | ||
| end |
There was a problem hiding this comment.
Sweet, didn't realise we could use a Lua file here!
There was a problem hiding this comment.
You can use a lua in every directory in the runtimepath. I think this was added in 0.5.0 (don't quote me on that ;) )
|
Lovely, thanks! |
|
I you are ok with it I can make other PRs to move the code to built in APIs. I'm quite familiar with neovim std library so I can help. My question would be do we have a clear minimum nvim version that we are targeting? For reference usually plugin authors supports the current stable release (now 0.10.*) and maybe the previous (now 0.9). |
|
So I've been trying to support as far back as I can without it causing headaches with lots of polyfilling and if statements in the code. There are some branches where I have to support two different APIs but not many. In hindsight I should've collected all version support branches in a specific module that makes it VERY obvious for when those things get removed. I worry about targeting stable because I'll be fine on Arch but our Ubuntu brothers and sisters might be a little upset about that if we don't have a good reason for it. Version churn for the sake of churn smells a little too npm for my liking so I try to avoid it. So, that is to say, lets try to support at LEAST the previous version, hopefully further back if possible. But this is all extremely subjective, we have to go with our gut. If we drop support for 0.8 in favour of 0.9 but the code looks identical if you squint and there's no benefit to the user, we shouldn't do it. If however we get things like first class Lua callbacks and can eliminate all the Vim Script bridge code with string interpolation all over the place, then we should DEFINITELY do that. Which is a thing I did a while back, dropping support for old versions in order to remove the hacky Vim -> Lua callbacks through strings was DEFINITELY worth it. This is just an example to highlight the sort of big improvements I think are definitely worth it, but we can judge it on a case by case basis. So, please feel free to drop usage of my custom Fennel in favour of built in APIs! If it will break support for older versions though let's weigh that up carefully against the need to move the version support line only when we need to, not because we want to. |
|
Ok, gotcha! I do believe that sticking with 0.8 is really a good sweet spot.
I do believe that centralize the polyfills can be a good thing to make the transition easier in the future. Also do we bother with native Windows support? Looking at the code we are very UNIX biased. I don't care about it but if you care I can also backport some of the latest |
|
We should try our best to support windows yeah, we definitely have windows
users. Really I'd like to get the test suite under GH actions and Linux,
Windows, Mac if possible. For now a best effort and hoping is all I've been
doing, but I can't work out how to run it and check things on windows
anyway right now, I'm useless with dev tooling on windows.
…On Mon, 23 Dec 2024, 12:17 Luca Saccarola, ***@***.***> wrote:
Ok, gotcha! I do believe that sticking with 0.8 is really a good sweet
spot.
In hindsight I should've collected all version support branches in a
specific module that makes it VERY obvious for when those things get
removed.
I do believe that centralize the polyfills can be a good thing to make the
transition easier in the future.
Also do we bother with native Windows support? Looking at the code we are
very UNIX biased. I don't care about it but if you care I can also backport
some of the latest fs.* stuff from neovim 0.10 that helps a lot with
Windows support.
—
Reply to this email directly, view it on GitHub
<#630 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AACM6XP7LJDQ75WBXLL7RHT2G75NTAVCNFSM6AAAAABTU6O23KVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKNJZGU4TIMBZGQ>
.
You are receiving this because you modified the open/close state.Message
ID: ***@***.***>
|
We are in the same boat I'm afraid |
Hi,
I've seen that in the README was stated that Conjure works with neovim 0.7 or above. I've found in
fs.fnlthatvim.fse.normalizeis been used, witch is a nvim 0.8+ feature so I've updated the minimum version required in the README andplugin/conjure.vim. With this version bump we unlock a lot of functionality that can replace some of the old batteries that were created.I've also took the liberty to convert the
plugin/conjure.vimto lua so we can usevim.notifyinstead ofechoerr.vim.notifycan integrate with other plugins and change the displaying of notifications (see example noice.nvim)