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

Remove plenary dependency?! #2552

Open
Conni2461 opened this issue Jun 7, 2023 · 21 comments
Open

Remove plenary dependency?! #2552

Conni2461 opened this issue Jun 7, 2023 · 21 comments
Labels
Projects

Comments

@Conni2461
Copy link
Member

and replace with vim internals (#2529 (comment)) or maybe we want to slim down plenary

@clason
Copy link
Contributor

clason commented Jun 7, 2023

I think it makes sense to tackle this here one module at a time; replacing a single stale plenary module with a maintained upstream module is a win itself, even if plenary itself remains a dependency.

I should also mention that it's perfectly fine to make feature requests (or PRs) to Neovim if one of the builtin modules is missing something that's needed here (and not too telescope-specific; otherwise just vendoring it here is best).

Of course, slimming down plenary and removing obsolete modules (or refactoring them based on upstream) is appreciated as well! I've heard repeatedly that the (understandable) lack of actual development there has dissuaded someone from contributing there. Something to consider is also to more strongly modularize plenary along the lines of https://github.com/echasnovski/mini.nvim (so each module can be installed independently as well).

From a brief grep of telescope, here's what's used, with possible replacements:

@Conni2461
Copy link
Member Author

Conni2461 commented Jun 7, 2023

  • plenary.busted / plenary.test_harness -> ?

@clason
Copy link
Contributor

clason commented Jun 7, 2023

I didn't list that because it's not an actual dependency for using telescope; you can handle this purely on the CI side (as we do in nvim-treesitter).

@Conni2461
Copy link
Member Author

Conni2461 commented Jun 7, 2023

I would have done it simiar to plenary.filetype replacement, step by step, especially because i dont have that much time.

I need to evaluate every module because i'm no longer that much up to date with neovim development and i also have to look into mini.nvim

Edit: strings upstreamen would probably be somewhat beneficial because its api-fast and calls c functions linetabsize_col would be way easier to do in nvim core

@clason
Copy link
Contributor

clason commented Jun 7, 2023

I need to evaluate every module because i'm no longer that much up to date with neovim development

Of course; This Is The Way. Let me know if you have any questions about the Neovim side (here or on Matrix).

Edit: strings upstreamen would probably be somewhat beneficial because its api-fast and calls c functions linetabsize_col would be way easier to do in nvim core

Yes, that is a good argument for having it in core; also consistent cross-platform handling of multibyte characters would be useful to have.

@max397574
Copy link
Contributor

max397574 commented Jun 7, 2023

I think plenary.strings could be quite easily upstreamed
it's nearly standalone
the only thing that's required from other modules is the path.sep
which imo could also be upstreamed since it's something you see really often in plugins and configs

I could open a pr for this

@clason
Copy link
Contributor

clason commented Jun 7, 2023

I don't think plenary.strings is upstreamable as-is. The way these things are done is that we first identify what we want to have in core -- including the API -- and then implement it. Requirements are often quite different for core than for a plugin.

(In particular, as @Conni2461 wrote, core should make use of Neovim's string handling functions in C as much as possible.)

@tjdevries
Copy link
Member

my only concern with the way things are for vim.system right now is I don't think it's as performant for doing line based items (which a lot of operations are for neovim). One thing that would be really awesome for vim.system would be some on_line or lines parameter or similar, and maybe some string wrangling in C to make things really zoom. But not sure, I'd have to do some benchmarks again.

@tjdevries
Copy link
Member

Would have really liked popup to have been fully fleshed out and made to match some of the vim apis, since that's a nice two-for-one kind of situation where a bunch of plugins can support vim and nvim with the same functions. But that doesn't look like it's going to happen (haven't seen any progress on that and I don't have any desire to drive those kinds of projects at the moment)

@tjdevries
Copy link
Member

Nice to see that maybe plenary can just be deprecated soon and a lot of the stuff has been upstreamed (and done better)

@clason
Copy link
Contributor

clason commented Jun 9, 2023

my only concern with the way things are for vim.system right now is I don't think it's as performant for doing line based items (which a lot of operations are for neovim). One thing that would be really awesome for vim.system would be some on_line or lines parameter or similar, and maybe some string wrangling in C to make things really zoom. But not sure, I'd have to do some benchmarks again.

That's not a bad idea; we have vim.iter now, and being able to feed stdout into that would be quite neat.

(I've also had situations where I'd have preferred a systemlist-type table output, but on synchronous jobs that's just a vim.split away, of course.)

@lewis6991
Copy link

my only concern with the way things are for vim.system right now is I don't think it's as performant for doing line based items (which a lot of operations are for neovim). One thing that would be really awesome for vim.system would be some on_line or lines parameter or similar, and maybe some string wrangling in C to make things really zoom. But not sure, I'd have to do some benchmarks again.

vim.system() is just a light wrapper on uv.spawn() with pipe management. It allows stdout to be provided as a basic handler which a line based handler could be written on top of. A line based handler doesn't necessarily need to be built in to vim.system directly.

@tjdevries
Copy link
Member

@lewis6991 from what I could tell, it always tries to put all the stdout into one variable at the end of execution? I have to go look again.

My main concern is when you do something like rg --files from home directory, i don't actually want to try and concat the string and save that all in memory (duplicated) again.

I can take a look again later tho.

@lewis6991
Copy link

lewis6991 commented Jun 9, 2023

It only does that if you don't provide your own handler. If you provide a handler of the form:

function stdout_handler(err, data)
end

That will be used instead of collecting the data into a variable.

The main point of vim.system() is just to provide automatic handling of the luv objects.

@tjdevries
Copy link
Member

ah, nice. thanks

@clason
Copy link
Contributor

clason commented Jun 11, 2023

Would have really liked popup to have been fully fleshed out and made to match some of the vim apis, since that's a nice two-for-one kind of situation where a bunch of plugins can support vim and nvim with the same functions. But that doesn't look like it's going to happen (haven't seen any progress on that and I don't have any desire to drive those kinds of projects at the moment)

I think that project has become a victim of Neovim's success: before the post-0.5 Cambrian Explosion of Lua plugins (and Bram's vim9script push), enticing Vimscript plugin devs with such an API seemed like a great way of growing the Neovim ecosystem. Now I could count the number of plugins profiting from this on one (saw-mill worker's) hand since they'd have to

  1. be originally written for Vim only;
  2. make heavy use of popup UI;
  3. are established and don't have any Lua peers;
  4. don't have full Neovim-specific alternate code paths for this already.

As it stands, it would make much more sense to add a Neovim-specific high(er) level vim.ui.popup() API in core (designed fresh with all the lessons learned).

@tjdevries
Copy link
Member

Yeah, but it does make situations like frameworks (denops comes to mind) where you want to have some shared API that works well in both quite nice.

(I just wanted to implement the vimscript APIs in Lua basically as wrappers around our code. A lot of it is quite nice for simple stuff that people generally do).

Yeah, it's probably not a huge win tho

@juanandresgs

This comment was marked as off-topic.

@clason

This comment was marked as off-topic.

@delphinus
Copy link
Contributor

@juanandresgs This is a bug by telescope-frecency.nvim. I fixed. Check it.

nvim-telescope/telescope-frecency.nvim#147

@mrcjkb
Copy link
Contributor

mrcjkb commented Sep 21, 2023

  • plenary.busted / plenary.test_harness

With nvim -l it's quite easy to use busted with Neovim as the interpreter, without plenary. See my blog post on the topic for details.

Some examples where this has been done:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
0.2
  
Awaiting triage
Development

No branches or pull requests

8 participants