Skip to content

Conversation

@Jint-lzxy
Copy link
Collaborator

This reverts commit 7056741.

Sorry for the noise but I'm a lil confused about this new option...? Like idk if the performance difference between these two is even that big, and maybe it's not actually working as expected?

This reverts commit 7056741.

Sorry for the noise but I'm a lil confused about this new option...? Like idk if
the performance difference between these two is even that big, and maybe it's
not actually working as expected?
@ayamir
Copy link
Owner

ayamir commented Jan 19, 2025

overwrite option is the only solution to delete pre-defined something, like I want to define edgy.nvim left region with NeoTree rather than NvimTree. Otherwise I need to use function to repeat lots of content the same with defaults.

@Jint-lzxy
Copy link
Collaborator Author

overwrite option is the only solution to delete pre-defined something

But I think this might break some users' expectations of the loader right 🤔 Also think I mentioned vim.NIL elsewhere too, it's like a user object so maybe we can use that to signal "erasure" of the field instead?

@ayamir
Copy link
Owner

ayamir commented Jan 26, 2025

Marking a option as nil is not enough, overwrite is an irreplaceable way to acheive delete and re-define.

@Jint-lzxy
Copy link
Collaborator Author

Marking a option as nil is not enough, overwrite is an irreplaceable way to acheive delete and re-define.

😭 srry if I wasn't super clear earlier! What I meant was we could let users pass vim.NIL inside the override table. Consider :h vim.NIL:

vim.NIL                                                              *vim.NIL*
    Special value representing NIL in |RPC| and |v:null| in Vimscript
    conversion, and similar cases. Lua `nil` cannot be used as part of a Lua
    table representing a Dictionary or Array, because it is treated as
    missing: `{"foo", nil}` is the same as `{"foo"}`.

To break it down, imagine the original table looks like this:

local orig = {
	top = {
		middle = {
			opt = "OKAY SOME OPTIONS",
			whatever = "BLA"
		}
	}
}

And the user's override is like this:

local override = {
	top = {
		middle = vim.NIL,
	},
}

Now, when we call our tbl_recursive_merge, here's what Neovim gives back:

{
  top = { 
    middle = vim.NIL 
  }
}

Then all we need to do is strip out the vim.NIL values in the recursive merge function and we're good to go! Also I've seen this (along with v:null) in tons of plugins, so I'd say it's a pretty "standard" approach and way more systematic. On top of that the main reason why I want to revert this first is "what's the point of setting overwrite=true just for this plugin?" I feel like our users' mileage might vary (possibly by a lot) otherwise.

@ayamir
Copy link
Owner

ayamir commented Mar 3, 2025

😭 srry if I wasn't super clear earlier! What I meant was we could let users pass vim.NIL inside the override table.

My real motivation for introducing this change is NOT to make a part of default configuration be nil, but to use the user's configuration to replace the part of default configuration. For this example, left option is exclusive for different configuration (nvim-tree or other file tree like neo-tree). If the default configuration for nvim-tree is not erased, the left part of edgy will show two file tree like this:

image

On top of that the main reason why I want to revert this first is "what's the point of setting overwrite=true just for this plugin?" I feel like our users' mileage might vary (possibly by a lot) otherwise.

The reason why I introduce it by adding a parameter to tbl_recursive_merge is whether overwrite or not is clearly for one plugin like the layout option of edgy.nvim.

Of course, the replace function can be impl by using return a function to achieve this, but it needs lots of redundant code.

@ayamir
Copy link
Owner

ayamir commented Mar 10, 2025

This option is useful for search.nvim too.

@Jint-lzxy
Copy link
Collaborator Author

My real motivation for introducing this change is NOT to make a part of default configuration be nil, but to use the user's configuration to replace the part of default configuration.

oh lol my bad I think I got what u meant now, srry for the confusion! I think passing a function as the value for that key should do the trick right? since it's our first branch it should def be respected

@ayamir ayamir merged commit 76d6274 into main Mar 28, 2025
4 checks passed
@ayamir ayamir deleted the revert/util-config-overwrite branch March 28, 2025 07:28
@ayamir
Copy link
Owner

ayamir commented Mar 28, 2025

I think passing a function as the value for that key should do the trick right? since it's our first branch it should def be respected

It's OK.

@TonyWu20 TonyWu20 mentioned this pull request Apr 3, 2025
TonyWu20 pushed a commit to TonyWu20/nvimdots that referenced this pull request Apr 3, 2025
…mir#1404)

This reverts commit 7056741.

Sorry for the noise but I'm a lil confused about this new option...? Like idk if
the performance difference between these two is even that big, and maybe it's
not actually working as expected?
@TonyWu20 TonyWu20 mentioned this pull request Apr 3, 2025
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