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

plugin with opt option install start dir #1038

Closed
glepnir opened this issue Aug 31, 2022 · 10 comments · Fixed by #1041
Closed

plugin with opt option install start dir #1038

glepnir opened this issue Aug 31, 2022 · 10 comments · Fixed by #1041
Labels

Comments

@glepnir
Copy link
Contributor

glepnir commented Aug 31, 2022

  • nvim --version: latest
  • git --version: latest
  • Operating system/version: macos
  • Terminal name/version: kitty

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

@glepnir glepnir added the bug label Aug 31, 2022
@EdenEast
Copy link
Contributor

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
Copy link
Contributor Author

glepnir commented Aug 31, 2022

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
Copy link
Collaborator

akinsho commented Aug 31, 2022

@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
Copy link
Contributor

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
Copy link
Contributor Author

glepnir commented Aug 31, 2022

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
Copy link
Collaborator

akinsho commented Aug 31, 2022

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
Copy link
Contributor Author

glepnir commented Aug 31, 2022

{ { 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
Copy link
Owner

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
Copy link
Contributor

EdenEast commented Sep 1, 2022

@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
Copy link
Owner

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.

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

Successfully merging a pull request may close this issue.

4 participants