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

feature: Improve dev option usability #993

Closed
1 task done
abeldekat opened this issue Aug 15, 2023 · 6 comments · Fixed by #1120
Closed
1 task done

feature: Improve dev option usability #993

abeldekat opened this issue Aug 15, 2023 · 6 comments · Fixed by #1120
Labels
enhancement New feature or request

Comments

@abeldekat
Copy link
Contributor

abeldekat commented Aug 15, 2023

Did you check the docs?

  • I have read all the lazy.nvim docs

Is your feature request related to a problem? Please describe.

A plugin can be configured in multiple locations, or have multiple specs in the same location.
In this request, the term snippet is used, referring to such a location.

When a plugin is configured across multiple snippets, and the user applies the dev marker on one of those snippets, the effect can be hard to reason about.

See the following issues:

  1. dev and dependencies #982
  2. dev and imports #985

The docs regarding dir and dev

Importing specs

opts, dependencies, cmd, event, ft and keys are always merged with the parent spec.
Any other property will override the property from the parent spec.

Plugin spec

dir: A directory pointing to a local plugin

dev: When true, a local plugin directory will be used instead. See config.dev

Configuration

  dev = {
    -- directory where you store your local plugin projects
    path = "~/projects",
    ---@type string[] plugins that match these patterns will use your local versions
    -- instead of being fetched from GitHub
    patterns = {}, -- For example {"folke"}
    fallback = false, -- Fallback to git when local plugin doesn't exist
  },

In the examples

-- local plugins need to be explicitly configured with dir
{ dir = "~/projects/secret.nvim" },

-- local plugins can also be configure with the dev option.
-- This will use {config.dev.path}/noice.nvim/ instead of fetching it from Github
-- With the dev option, you can easily switch between the local and installed
-- version of a plugin
{ "folke/noice.nvim", dev = true },

Analyzing the concept

The dir and dev keywords are conceptually the same.
The dev keyword implicitly sets the dir, according to:

dir = config.dev.path

When applied on an individual snippet, the dir is only changed on that snippet, and can be easily overridden

When using config.dev in require("lazy").setup(), the dev option triggers the calculation of the dir in all matching snippets.

Examples: Order matters

dev on first snippet

local plugins = {
  { "lewis6991/gitsigns.nvim", opts = {}, dev = true },

  -- TESTING:
  -- ATTENTION: `dir` override, now defaults to `root`
  { "lewis6991/gitsigns.nvim" },
}
local plugins = {
  { "lewis6991/gitsigns.nvim", opts = {}, dev = true },

  -- fallback, no new `dir`, `dir` is `config.dev.path`
  { "gitsigns.nvim" },
}

The results would be the same if the first line is changed into:

{ "lewis6991/gitsigns.nvim", opts = {}, dir = "~/projects/gitsigns.nvim" },

dev on second snippet

local plugins = {
  { "lewis6991/gitsigns.nvim", opts = {} },

  -- TESTING:
  -- new `dir` overrides, `dir` is `config.dev.path`
  { "lewis6991/gitsigns.nvim", dev = true },
}
local second = {
  { "lewis6991/gitsigns.nvim", opts = {} },

  -- ATTENTION: no new `dir`, old dir defaults to `root`
  -- BUG: this is a valid spec, dir should have been calculated.
  { "gitsigns.nvim", dev = true },
}

Note the line containing "BUG" in the last code fragment.
The snippet is a valid short named specification, thus the dev marker should have triggered the calculation of dir
The result differs if the last line is changed into:

{ "gitsigns.nvim", dir = "~/projects/gitsigns.nvim" }

With this change, dir is applied correctly

Usage

dir property

  1. The user starts working on a new local plugin.
  2. The user quickly wishes to connect to a plugin somewhere on his system
  3. The user configures a plugin to be managed externally, for example by nixos

The user has no specific need to maintain multiple snippets for this plugin.
The user makes sure his snippet is not overridden with another dir.

config.dev on setup

  1. The user wishes to easily maintain plugins in one central local location.
  2. Configuration should be possible without cluttering individual snippets with a marker and without thinking about any overriding.

Describe the solution you'd like

  1. Keep the dir property as is.
  2. Remove the dev marker from the plugin spec.
  3. Only support config.dev on setup.
  4. Warn when a plugin matches with config.dev.patterns, and also has a dir specified
  5. Dedicate a small section in the doc, centralizing how local plugins can be used.

A technical thought

Currently, config.dev.patterns is handled in Plugin.add. Perhaps that code could be handled in Plugin.fix_disabled, a sanitation step. In that case, Plugin.fix_disabled needs to be renamed.

Update 2023.09.08: I don't think my technical suggestion above is valid. The dir property is needed when importing specs.

Describe alternatives you've considered

Never overwrite a dir once its set on a snippet. Requires changing Plugin.merge

Additional context

See the following issues:

  1. dev and dependencies #982
  2. dev and imports #985
@mehalter
Copy link

Just to expand on the alternative that is described here (which I think is a really good move forward) is the general explanation of that approach. Basically through these "snippets" (as used in the original issue body)/partial plugin specifications, as they are merged together it seems logical and intuitive that only things explicitly added in the specs by the user should be considered until the very end where lazy has as much user information as it can before making implicit decisions. If the user does not explicitly add dir then it should not be overwritten implicitly as more specs are added. Resolving this for all properties of the plugin specs (snippets) would also resolve this issue since then this weird phantom dir that is never being explicitly set in some of the examples wouldn't come through and clobber the dir set by dev = true.

This would most likely involve resolving the specs as the user provides them before calculating stuff like dir changing based on dev = true for instance. This seems like while it would improve the understanding to the user of what changes they are actually making, this would also help improve performance since you wouldn't need to calculate all of these things n times where n is the number of snippets that define specs for the same plugin.

note: this performance point is coming completely from someone who does not have a full understanding of the current processes in place in lazy.nvim, but is just intuition from the order in which things are happening in the results. If this intuition is incorrect I apologize and that point can be ignored.

Thanks so much to folke for all of the effort and amazing work put into lazy.nvim and huge thanks to @abeldekat for their help organizing the thoughts of these two issues. ❤️

@abeldekat
Copy link
Contributor Author

abeldekat commented Aug 15, 2023

Hi @mehalter,

Thanks for the response, and for the term 'partial plugin specifications'. I think partial covers the intent better than snippet.

...that only things explicitly added in the specs by the user should be considered until the very end where lazy has as much user information as it can before making implicit decisions...

This is already the case! Only the dev marker could be improved in that aspect.

... If the user does not explicitly add dir then it should not be overwritten implicitly as more specs are added...

Agree.

...Resolving this for all properties of the plugin specs...

I don't think there is anything to resolve, except for the handling of the dev marker.

.. this would also help improve performance ..

Perhaps a little, but only when actually using the dev marker, or config.dev at setup. The performance of lazy.nvim will not be affected by the changes proposed here.

...If this intuition is incorrect I apologize and that point can be ignored...

No need to apologize in my opinion. I appreciate your commitment!

To summarize, I think lazy.nvim already operates as you expect. Changing the handling of the dev option would be relatively simple.

@mehalter
Copy link

Awesome, thanks for the explanation @abeldekat it doesn't quite work the way I expect since the dev and dir fields do not follow these rules. I just wanted to make sure that the general operation is rigorously defined to make sure if we need to just fix dev and dir or if this is a deeper problem across the other fields but just hasn't come up yet. It sounds from your response that it is isolated to dev and dir which is good 😀

I also agree that if it does only affect dev and dir then this would only improve performance when that is used 😄

@s1n7ax
Copy link

s1n7ax commented Oct 8, 2023

Don't understand the most of mentioned here. But just wondering if there is a way to use dev = true if dir exists.

The issue I'm facing is, I use a plugin to manage all my luasnips. I frequently update them so I have set dev = true and set the path using dir. I recently started using devcontainers so I'm setting up containers and neovim for each and every project. Now, in containers, I don't have luasnips local directory. So, if I want to use snips inside containers, I have to change the local config to dev = false and then run neovim in container.

May be a solution to this can be included too.

Edit:

Now that I'm thinking about it, I can just do this,

Edit:

Tested it and working

local plugin_path = '~/Workspace/nvim-snips'

local M = {
	's1n7ax/nvim-snips',
	name = 'snips',
	dir = plugin_path,
}

if vim.fn.isdirectory(plugin_path) == 1 then
	M.dev = true
	M.dir = plugin_path
end

return M

@folke folke closed this as completed in 3dc413d Oct 15, 2023
folke added a commit that referenced this issue Oct 15, 2023
…om the plugin's old dir. Show an error in this case. Fixes #993
@folke
Copy link
Owner

folke commented Oct 15, 2023

Just pushed an update where both dir and dev now works as expected.
Whenver the dir property of a plugin changes, a warning will be added to :checkhealth.

One important caveat though. During spec parsing, some plugins may already get partially loaded if they are require()d inside the spec files. LazyVim is an example of this.

When we detect a dir change for a plugin that is already partially loaded, the new dir will NOT be used and an error will be shown. Loading files from multiple different directories for the same plugin is not something we want.

@abeldekat
Copy link
Contributor Author

Thanks!

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

Successfully merging a pull request may close this issue.

4 participants