wbthomason / packer.nvim

A use-package inspired plugin manager for Neovim. Uses native packages, supports Luarocks dependencies, written in Lua, allows for expressive config
MIT License
7.75k stars 261 forks source link

plugin with opt option install start dir #1038

Closed glepnir closed 1 year ago

glepnir commented 1 year ago

Steps to reproduce

use({
  'kristijanhusak/vim-dadbod-ui',
  cmd = { 'DBUIToggle', 'DBUIAddConnection', 'DBUI', 'DBUIFindBuffer', 'DBUIRenameBuffer' },
  config = conf.vim_dadbod_ui,
  requires = {'tpope/vim-dadbod', opt = true },
})

Actual behaviour

vim_dadobd installed in start_dir

Expected behaviour

install in opt_dir

EdenEast commented 1 year ago

There are two types of plugin specs, Simple specs that are just a string and Complex ones that are a table containing optional values.

requires takes either a string for a single simple plugin or a table for a list of plugins. Since you are passing a table, packer is using that table as a list of plugins to require. Wrapping your complex plugin spec in another table will create a list of one complex plugin. This will solve your issue.

use({
  'kristijanhusak/vim-dadbod-ui',
  cmd = { 'DBUIToggle', 'DBUIAddConnection', 'DBUI', 'DBUIFindBuffer', 'DBUIRenameBuffer' },
  config = conf.vim_dadbod_ui,
  requires = {{'tpope/vim-dadbod', opt = true }},
})
glepnir commented 1 year ago

I used this before. but i don't know this logic if its a table with opt. it should be in opt dir. why it must be work with table in table? also test with


use({ 'plugin', opt = true})
akinsho commented 1 year ago

@glepnir the original example for the issue description would probably never worked exactly right, i.e. it would likely have always been an issue. I think packer already goes overboard to provide loads of variations for input, e.g. you can supply requires = {"a/list", "of/strings"} or requires = "single/string" or requires = {{'more/complex', config = "blah"}, {'list/of', setup = 'foo'}} I think it's reasonable to not try to cover every single possible mechanism and just have a list

The example you mention above is totally different. use({ 'plugin', opt = true}) is a single argument directly passed to use whereas what you are trying to do is pass a child plugin spec inside a requires key.

Packer would then have the fun job of disambiguating between an "object" or a "list" which tbh can be done with vim.tbl_islist but I really think for sanity and testing purposes the current number of patterns is more than enough.

EdenEast commented 1 year ago

I agree with @akinsho, the manage function is already complex enough. If anything packer should become simpler as I have found that this is an area where there is a lot of confusion.

glepnir commented 1 year ago

If a function would become more complex. But there are still situations that cannot be handled. I'm more inclined to think that the design of this function is wrong . And I think it's a long-standing bug? It's just that I discovered it recently. I don't understand this logic. Because the function will get complicated without fixing the bug. Will judging these situations be complicated? I don't think so . Not a big deal. If you don't think it's necessary. I can close. Or I can make a plugin manager of my own. that would be a good start too

requires = {"a/list", "of/strings"} or requires = "single/string" or requires = {{'more/complex', config = "blah"}, {'list/of', setup = 'foo'}}
akinsho commented 1 year ago

But there are still situations that cannot be handled. I'm more inclined to think that the design of this function is wrong

I'm not sure this logic is valid personally, I think if you invert this logic then every function that doesn't handle every scenario is wrong?

Anyway I don't speak for packer, I was simply chiming in to suggest that this is to my mind a very minor issue, all you need to do is wrap the requires with {} and carry on tbh. I don't think this needs to be fixed, but @wbthomason might disagree 🤷🏿‍♂️, certainly not more urgently than other things.

If you want to make a plugin manager this is very much your prerogative, a bit tangential here though. If other people think this is worth fixing, then feel free to raise a PR.

glepnir commented 1 year ago

{ { plugin opt = true} } ...don't know this logic .if only have one you must be write like this. it's wired 😄 . Project is a little bloated. So I probably won't look at the relevant code to send any pr .closed

wbthomason commented 1 year ago

I agree with @akinsho and @EdenEast here: early influence on packer pushed us toward going to significant lengths to allow probably too many different input formats, which has increased complexity/bloat and led to annoying edge cases like this. I would say that this is "working as expected", for the reason that @EdenEast mentions, but I agree that it's annoying.

I took a look at a fix and it seemed simple enough without adding much more complexity to manage, so I made #1041. If that fixes this issue for you, that's great.

In general, I do want to slim packer. It's become a large, fairly unpleasant codebase due to feature creep. I've made some headway on this in the refactor/packer-v2 branch, but that's been stalled because I haven't had time for OSS work in quite a while. I also still owe @akinsho and others a roadmap for the rewrite to help distribute the work. If you're interested in contributing to this semi-clean-slate work, please let me know.

EdenEast commented 1 year ago

@wbthomason I would like to help contribute to this cleanup / v2 effort. I am able to put resources into OSS and would like to contribute to packer.

wbthomason commented 1 year ago

Thank you! I finally got around to writing a very first-draft roadmap for the rewrite: https://github.com/wbthomason/packer.nvim/blob/refactor/packer-v2/ROADMAP.md

It needs fleshing out and probably other revisions, but hopefully this is enough to start a conversation on how others can help contribute to the rewrite effort.