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

feat: support explicit lazy=false #122

Merged
merged 1 commit into from
Jan 1, 2025
Merged

Conversation

zoriya
Copy link
Contributor

@zoriya zoriya commented Dec 25, 2024

When specifying explicitly lazy = false but still keeping a lazy handler (keys for example), the lazy value is overriden to true.

My use case of specifying lazy = false is to configure plugins in the start directory of my packpath. For example:

return {
	{
		"oil.nvim",
		lazy = false,
		load = function() end,
		keys = {
			{ "-",    "<CMD>Oil<CR>", desc = "Open parent directory" },
			{ "<BS>", "<CMD>Oil<CR>", desc = "Open parent directory" },
		},
		opts = {
			skip_confirm_for_simple_edits = true,
			view_options = {
				show_hidden = true,
			},
			keymaps = {
				["<BS>"] = "actions.parent",
			},
		},
		after = function(plug)
			require("oil").setup(plug.opts)
		end
	}
}

Copy link
Contributor

github-actions bot commented Dec 25, 2024

Review Checklist

Does this PR follow the Contribution Guidelines? Following is a partial checklist:

Proper conventional commit scoping:

  • For example, fix(context): some bugfix

  • Pull request title has the appropriate conventional commit prefix.

If applicable:

  • Tested
    • Tests have been added.
    • Tested manually (Steps to reproduce in PR description).
  • Updated documentation.

@HeitorAugustoLN
Copy link
Contributor

I've been setting this:

{
  "nvim-treesitter",
  after = function()
      vim.opt.runtimepath:prepend(vim.fs.joinpath(vim.fn.stdpath("data"), "site"))
      require("nvim-treesitter.configs").setup({
          highlight = { additional_vim_regex_highlighting = { "ruby" }, enable = true },
          indent = { disable = { "ruby" }, enable = true },
          parser_install_dir = vim.fs.joinpath(vim.fn.stdpath("data"), "site"),
      })
  end,
  cmd = { "TSInstall", "TSUpdate", "TSUpdateSync" },
  event = { "BufNewFile", "BufReadPost", "BufWritePost", "DeferredUIEnter" },
  lazy = vim.fn.argc(-1) == 0,
}

to not lazy-load treesitter if I open a file directly, and it is working just fine.

And I don't think you should be setting a lazy handler if you are really don't want to lazy-load the plugin, it is just better to set the keymaps in the after function

@mrcjkb
Copy link
Member

mrcjkb commented Dec 28, 2024

Hey 👋
Thanks for the PR. 🙏

As @HeitorAugustoLN mentioned, the keys table is not meant to be used as a DSL for defining keymaps.
If not used for lazy loading, it just adds overhead (to create the loading trigger keymaps).
I'd also recommend just to use vim.keymap.set.

I'm not sure how I feel about this suggested change in yet. Setting lazy = false and defining lazy-loading handlers is a conflict. But there may be a use case for wanting to override lazy loading under certain conditions. @HeitorAugustoLN in your example, I believe it's one of the events that triggers, not vim.fn.argc(-1) == 0?

@zoriya please also see the review checklist in the github-actions comment (if there is a good use case for this behaviour).
Regarding the commit prefix: This would be a feat, not a fix, as lazy = false was never intended.

@HeitorAugustoLN
Copy link
Contributor

HeitorAugustoLN commented Dec 28, 2024

in your example, it's one of the events that triggers, not vim.fn.argc(-1) == 0?

just tested it, and yeah it is. i didn't thought so. But after testing out @zoriya modifications, looks like I gained a bit of startuptime

startuptime-lazy.log
startuptime-nolazy.log

Measured with:

nvim flake.nix --startuptime startuptime-lazy.log

with lazy = vim.fn.argc(-1) == 0

nvim flake.nix --startuptime startuptime-nolazy.log

with lazy = vim.fn.argc(-1) == 0 commented out

EDIT: Measured startuptime with the current latest lz.n and there really no difference with lazy = vim.fn.argc(-1) == 0 or not

@mrcjkb
Copy link
Member

mrcjkb commented Dec 28, 2024

The difference is too small to be regarded as a benchmark, but thanks for the insight.
You presented a valid use case for wanting to explicitly set lazy = false.

@zoriya
Copy link
Contributor Author

zoriya commented Dec 28, 2024

If you want I can update the PR to not setup lazy handlers when lazy = false. This would solve the overhead issue.

I'll update the PR with the checklist next week, I'm on vacation right now.

@mrcjkb
Copy link
Member

mrcjkb commented Dec 28, 2024

If you want I can update the PR to not setup lazy handlers when lazy = false. This would solve the overhead issue.

No, that's fine. It would increase complexity and likely introduce unwanted behaviour.

@HeitorAugustoLN
Copy link
Contributor

If you want I can update the PR to not setup lazy handlers when lazy = false. This would solve the overhead issue.

No, that's fine. It would increase complexity and likely introduce unwanted behaviour.

Yeah, agreed.

@mrcjkb mrcjkb enabled auto-merge (squash) December 29, 2024 01:54
HeitorAugustoLN added a commit to HeitorAugustoLN/nvim-config that referenced this pull request Dec 31, 2024
- Use forked lz.n from zoriya/lz.n pending upstream merge
- Configure lz.n package to use the input package

Using forked version that supports `lazy = false` with other lazy handlers set up
until changes are merged upstream in nvim-neorocks/lz.n#122
@zoriya zoriya changed the title Fix explicit lazy=false feat: support explicit lazy=false Jan 1, 2025
auto-merge was automatically disabled January 1, 2025 20:46

Head branch was pushed to by a user without write access

@zoriya zoriya force-pushed the master branch 2 times, most recently from 32b764e to bdcf367 Compare January 1, 2025 20:48
@zoriya
Copy link
Contributor Author

zoriya commented Jan 1, 2025

I added a test & fixed the conventional commit

@mrcjkb
Copy link
Member

mrcjkb commented Jan 1, 2025

I added a test & fixed the conventional commit

Thanks 🙏

@mrcjkb mrcjkb merged commit 5a6d5fa into nvim-neorocks:master Jan 1, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants