feat: support dev.path as function

[atusy]

In some case, dev.path .. plugin.name is not enoguh.

For example, when using ghq to manage projects, plugin directories may vary by owners of the plugins.

With this change, users can do something like below

require("lazy").setup("plugins", {
  dev = {
    path = function(plugin)
      -- ghq
      local path, cnt = string.gsub(plugin.url, "^https://(.*)%.git$", "~/ghq/%1")
      if cnt == 1 then
        return path
      end

      -- fallback to default
      return "~/projects/" .. plugin.name
    end,
  },
})

[BirdeeHub]

#1276

Im rooting for you I think it is better than my dev.extra_paths

[BirdeeHub]

I think this is broken actually, it does not change the conditional before it in the plugin.lua file, which would try to convert the function to a string.

[Shougo]

Well, you should upload the example.

[BirdeeHub]

Still writing it. But check the conditional right before your change to plugin.lua

vim.fn.isdirectory(Config.options.dev.path .. "/" .. plugin.name) == 1)

[Shougo]

It means plugins may access dev.path as string?

[BirdeeHub]

right. if dev == true, and fallback is true it will then try to implicity convert a function to a string and throw an error.

[Shougo]

I don't know plugins want to access dev.path directly. But if so, it is not backward compatible.
It should be documented.

[BirdeeHub]

It will throw an error 2 lines before your change to plugin.lua if dev == true and fallback == true and dev.path is a function

[BirdeeHub]

E5108: Error executing lua [string ":lua"]:1: attempt to concatenate field 'stdpath' (a function value)
stack traceback:
        [string ":lua"]:1: in main chunk

Trying to concatenate a function gives this error the above is the output of :lua print(vim.fn.stdpath .. "something")

[BirdeeHub]

Should I add it to mine?

[Shougo]

OK. I get it.

  -- dev plugins
  if
    plugin.dev
    and (not Config.options.dev.fallback or vim.fn.isdirectory(Config.options.dev.path .. "/" .. plugin.name) == 1)
  then
    dir = Config.options.dev.path .. "/" .. plugin.name
  elseif plugin.dev == false then
    -- explicitely select the default path
    dir = Config.options.root .. "/" .. plugin.name
  end

Config.options.dev.path is used in two places. It seems the PR's bug.

[BirdeeHub]

Ok. So should I add the option to my commit? or do you want to fix yours? The exerpt I put above works and is about as performant as it could be and allows fallback functionality to work even if the option was a function.

[BirdeeHub]

you can copy it into yours if you want, im not picky about where the change goes

I want some form of mutliple paths in there though so if you close yours im going to want to add it to mine

[BirdeeHub]

I feel that it would be less confusing if you added the fixed version to yours but either works, I will happily add it to mine and update the message and title and stuff.

[BirdeeHub]

lol i accidentally had the type function's parenthesis in the wrong spot but yeah now its right XD

[BirdeeHub]

Only question is, should resolveDevPath return an empty string or should it return nil if everything fails?

Will it throw a different error anywhere if dir is nil rather than an incorrect path? previously the fallback case was to just send the incorrect path, so to be completely accurate we should send back an empty string, however i think nil is more clear.

Edit: Testing says failure case should return empty string

[BirdeeHub]

Ok. Fully tested. The following code behaves exactly as expected in all scenarios. I have determined that setting the path to nil causes the dev.fallback option to not be respected although I am not sure why. I only know that if resolveDevPath can return nil and the function I provide returns nil, it causes plugins to be downloaded regardless of fallback setting. Thus resolveDevPath must return an empty string if options fail.

[BirdeeHub]

OH WAIT THIS ISNT YOUR PR THIS IS SOMEONE ELSES

[BirdeeHub]

Well, if I dont get a reply like, soon, Im going to just stick it in my PR cause I wrote it at this point

[BirdeeHub]

@Shougo I thought this was your pr lmao

[BirdeeHub]

I like your idea, but it does not work.
The following would achieve the desired effect.

Alternatively, you can close the PR and I will add it to my commit #1276

replace the block containing my comment pointing out the bug with the following:

  -- dev plugins
  local function resolveDevPath(lazyPlugin)
    local path = nil
    if type(Config.options.dev.path) == "string"
      and vim.fn.isdirectory(Config.options.dev.path .. "/" .. lazyPlugin.name) == 1
    then
      path = Config.options.dev.path .. "/" .. lazyPlugin.name
    elseif type(Config.options.dev.path) == "function" then
      path = Config.options.dev.path(lazyPlugin)
      -- norm will throw if provided nil, so we apply it when we check if the result was valid
      if type(path) == "string" then
        path = Util.norm(path)
      else
        path = nil
      end
    end
    if path == nil then
      if Config.options.dev.fallback then
        path = Config.options.root .. "/" .. lazyPlugin.name
      else
        -- should return empty string if git fallback is false. Otherwise it will not
        -- respect the fallback option's value and download anyway.
        path = ""
      end
    end
    return path
  end

  if plugin.dev then
    dir = resolveDevPath(plugin)
  elseif plugin.dev == false then
    -- explicitely select the default path
    dir = Config.options.root .. "/" .. plugin.name
  end

[BirdeeHub]

Will throw an error trying to concatenate a function on this line if fallback and dev are true

[BirdeeHub]

shoot.... I can actually add the changes but i didnt realize that.

Wait maybe i cant it wont let me push it

Ehhhh nvm I dont know how to do that. or if I can

[atusy]

Calm down. I will take a look today or maybe tommorow.

[BirdeeHub]

:) sorry I was impatient, but yeah I fixed it for you. I am running it in my fork's main branch (my pr is in a different one) and it works great. I even changed the wrapper for lazy in my nixCats repo to use the new option instead of my dev.extra_paths I had before.

[atusy]

No worries. Thank you for reporting the bug.

[atusy]

@BirdeeHub I made the fix. Hope it works.

[mehalter]

Just tested the latest commit here and it works beautifully! Incredible to have support for arbitrary logic for the development path. Also improves the ability to develop on Lazy as well if you use something like an environment variable for specifying the lazy path! Tested with the following function (I use a $GIT_PATH environment variable for my ghq path):

  dev = {
    ---@param plugin LazyPlugin
    path = function(plugin)
      -- use LAZY environment variable for lazy development
      if vim.env.LAZY and plugin.name == "lazy.nvim" then return vim.env.LAZY end

      -- resolve git path if available or fallback to ~/projects
      local dir = plugin.url:match "^https://(.*)%.git$"
      return dir and vim.env.GIT_PATH and vim.env.GIT_PATH .. "/" .. dir or "~/projects/" .. plugin.name
    end,
  },

[BirdeeHub]

Hmmmm. Logic checks out. It looks much nicer than my suggestion. It will throw if the function returns something that isnt a string, however, that is kinda the desired behavor anyway so I suppose you are right that fixing that part is unnecessary.

So yeah great work, good fix.

[mehalter]

Yeah, this code above will only return a string just make sure that it doesn't run into those cases. Either the git path if it can resolve one or just return the default ~/projects/<plugin.name>

[BirdeeHub]

so, if the function itself that you provide as an option returns a non-string it throws an error error because norm throws if provided a non-string.

It will still respect fallback setting though as long as the function returns a string.

We only want the function in the option to return a string though. Only difference between this fix and my suggestion is that in the case of the function not returning a string, this one throws throws, and mine forwards the throwing to the code loading the plugin by returning an empty string in that case. And since that is a failure case anyway, that is ok.

To fix this behavior, the following would be needed, however it isnt necessary.

  if plugin.dev then
    local dir_dev
    if type(Config.options.dev.path) == "string" then
      dir_dev = Config.options.dev.path .. "/" .. plugin.name
    else
      dir_dev = Config.options.dev.path(plugin)
      -- you would need to norm after checking if string to avoid error being thrown when function returns a non-string
      if type(dir_dev) == "string" then
        dir_dev = Util.norm(dir_dev)
      else
        dir_dev = ""
      end
    end
    if not Config.options.dev.fallback or vim.fn.isdirectory(dir_dev) == 1 then
      dir = dir_dev
    end

So yeah, you could make this unnecessary fix, or not, it does not matter.

[mehalter]

I'm not sure I follow. Are you just saying that the dev.path function needs to return a string?

The dev.path function I shared above always returns a string so it should be good right?

[BirdeeHub]

Yeah as long as it always returns a string you are fine. If it doesnt then it will throw an error when it tries to norm it.

Again, unsure if fixing that behavior is necessary, but if OP wanted to, they could add that extra check to fix it.

[mehalter]

Yeah the function I shared above only returns a string. There is no case where it will return a non-string so it will always work. Nothing to fix?

[BirdeeHub]

correct nothing to fix as far as your function is concerned. Im just saying that one could handle the failure case in the proposed PR slightly more gracefully if desired, but again, it would throw an error at some point either way if not provided a string so maybe throwing earlier is better.

But yeah if the function does not provide a string it will throw, and halt execution of your config file, whereas if it passed an empty string in that case, it would load the plugins but just fail for that one. Small difference in the end either way.

[BirdeeHub]

So, yeah, again, the extra check I just mentioned isnt needed, it just would match the original behavior in the failure case slightly better. Which, again is a failure case and would throw an error eventually anyway.

[mehalter]

  dev = {
    ---@param plugin LazyPlugin
    path = function(plugin)
      -- use LAZY environment variable for lazy development
      if vim.env.LAZY and plugin.name == "lazy.nvim" then return vim.env.LAZY end

      -- resolve git path if available or fallback to ~/projects
      local dir = plugin.url:match "^https://(.*)%.git$"
      return dir and vim.env.GIT_PATH and vim.env.GIT_PATH .. "/" .. dir or "~/projects/" .. plugin.name
    end,
  },

There are 3 cases here (for the above function):

1. vim.env.LAZY is set and lazy.nvim is set to dev in which case it returns vim.env.LAZY (which environment variables are strings)
2. dir is matched and vim.env.GIT_PATH is set (not-null and therefore a string) so it returns vim.env.GIT_PATH .. "/" .. dir (which is a string)
3. the above conditional doesn't match and it returns "~/projects/" .. plugin.name (which is a string)

Yeah I think requiring a string is a completely fine assumption and in the code we can enforce this to some degree with lua language server annotations where its dev.path is typed to string|fun(plugin: LazyPlugin): string which should be sufficient to have type safety in the lazy.nvim codebase and then anything provided that doesn't match that typing is just a user error.

[BirdeeHub]

correct.

Your function always returns a string and thus would be fine.

You seem to grasp the issue I am mentioning and also why it isnt important

It is simply a matter of how hard it throws when not provided a string by the function. It will either throw right then in the case of the PR which may be the desired behavior, or simply not find the plugin if the extra fix I just mentioned in #1157 (comment) were to be added.

Either way, en error would be thrown eventually so it doesnt really matter, it would simply change at what stage the error happens.

Again, overall a great change, and it would be fine with or without this extra proposed change to it, and not having the extra check would technically be better for performance in the case that the function you provide as an option always returns a string. So it could go either way as to what would be better and it is ok with me either way.

[BirdeeHub]

So yeah, @atusy great work, I have a nitpick above if you care or think it matters, but otherwise, great work, now works as expected.

[BirdeeHub]

Now we just need a folke to accept the stuff XD

[atusy]

I would throw the error when Config.options.dev.path() does not return string.
So let me keep the current implementation, and wait for the folke's review.

BTW, I made another fix which applies Util.norm() also when Config.options.dev.path is a string.

I will rebase and force push the fixup commits later.

[BirdeeHub]

nice. Just figured id point it out just in case you didnt notice :) But yeah as far as im concerned, this is fixed, and working in my fork of lazy.nvim in the main branch.

And I think yours already does apply norm if its a string. You added that in the config.lua file
You already do this in config.lua so you do call norm if it was a string already.

  if type(M.options.dev.path) == "string" then
    M.options.dev.path = Util.norm(M.options.dev.path)
  end

So yeah, as far as i can tell you already call norm in all the relevant places, the only nitpick I have is the thing I already mentioned.

I think config.lua is the correct place to call norm if it was a stringto begin with because then it only happens once and not once per plugin. And you did that already so, nice.

[BirdeeHub]

So yeah, i dont think you should call norm in any more places than you do already.

You already normalized all the possible inputs. You just normalize the value when it was already a string within the config.lua file rather than the plugin.lua (which is the correct spot for that, so good job on that part, nailed it)

I would probably suggest leaving it exactly as is at this point.

[BirdeeHub]

Hmmm you moved the norm actually. It does now address the error if the function returns a non-string, however now it runs norm 1 time for every plugin regardless of whether the option was a function or not, thus that means you could remove the norm from config.lua if you wanted. If i had to pick it would be having it the way you had it before in the 2nd commit but thats ok, either is fine at this point.

I would say that norming the option if it is a string in config.lua and having the norm directly norming the output of the function was better than the third commit where you moved the norm to when you actually assign the value to dir despite the fact that it now handles errors more gracefully, but in the grand scheme of things, running 1 extra norm function per plugin is very small.

[BirdeeHub]

If I had to choose between commit 2 and commit 3 Id pick 2 for that reason though. You already norm the option if it was a string in config.lua and so now you are running 1 extra norm per plugin. So I would let it throw the error and just norm the output of the function at this point, just in the interest of not norming things that have already been normed. You could check if the value was a string as I suggested in my nitpick, or leave it exactly like commit 2. Norming at the end like this leads to an extra norm per plugin regardless of how you set dev.path either as a string or a function, whereas before it only did it 1 time at the start if string and every time if function.

[BirdeeHub]

So yeah, in short, the first fix fixed it, the next one fixed the error if the function does not return a string but will now norm the thing once at the start and then also once per plugin if dev.path was a string. Throwing the error was better.

Thats why in my nitpick I just did

if type(dev_dir) == "string" then dev_dir = Util.norm(dev_dir) else dev_dir = "" end

#1157 (comment)

rather than trying to move the norm to the end like you did in the 3rd commit. That way it only adds a single check if it is a string if dev.path was a function, and does not impact performance of the normal option at all.

So yeah, stick with the second commit, or add this check to the second commit, either way, the 2nd commit was better.

[atusy]

You're right. I removed the third commit and squashed the rest.

[BirdeeHub]

Beautiful. Great working with you ❤️

[BirdeeHub]

When I said it was a nitpick I meant it XD I was just making sure you were aware

[folke]

Thanks!