add postfix to default snip_env & use vim.tbl_extended

[hiberabyss]

[L3MON4D3]

Might've been a bit hasty in asking for a PR :sweat_smile: What didn't work about just adding postfix to defaults?

[hiberabyss]

@L3MON4D3 Will have following errors:

[hiberabyss]

Might've been a bit hasty in asking for a PR 😅 What didn't work about just adding postfix to defaults?

Maybe we could have an option to control whether replace or extend, since extend should be desired for most users.

[L3MON4D3]

@L3MON4D3 Will have following errors:

Ahh, changing local snip = require("luasnip").snippet to local snip = require("luasnip.nodes.snippet").S in postfix.lua should fix that

[L3MON4D3]

Might've been a bit hasty in asking for a PR sweat_smile What didn't work about just adding postfix to defaults?

Maybe we could have an option to control whether replace or extend, since extend should be desired for most users.

Yeah okay, I can see the sense in that. Do add this as a different option though, this doesn't have to be a breaking change. (snip_env_extend? extend_snip_env?)

[hiberabyss]

Yeah okay, I can see the sense in that. Do add this as a different option though, this doesn't have to be a breaking change. (snip_env_extend? extend_snip_env?)

Update PR with snip_env_extend and also fix postfix.lua.

[L3MON4D3]

Mhmm, I dislike a bit that snip_env_extend, a config option, controls the behaviour of another (though it is clear that they are related) I can see two other approaches:

• set some key in the table passed as snip_env to indicate that we want to extend the default
• make snip_env_extend also accept a table, for creating the effective snip_env we could first set it to user_conf.snip_env (if that is set), and then force-extend the result with user_conf.snip_env_extend

I prefer 2, but am not completely happy with it, since it's very close to having the same functionality twice, which feels a bit confusing. (although, they're not thaaaat close) WDYT?

Also, I think we should change the name to snip_env_override, "extend" suggests (to me at least) that the defaults won't be overriden, but that's definitely the desired behaviour here.

[hiberabyss]

@L3MON4D3 How about add a with_default like function? The function will extend default with user conf. Could be used for any conf.

[L3MON4D3]

So we'd expose ls.config.with_default(user_conf) and it extends the default.. Yeah that would work, but I think this isn't applicable to any other config-option, snip_env is the only one where we'd want to extend/override the default (okay, technically the autocommand-names can be extended, but I don't think this is desirable).

Having a more general mechanism that only really works with one option doesn't seem like the way to go

[leiserfg]

IMHO either snip_env gets always merged with the defaults or we expose the defaults so the users do the merge themselves. Adding two parameters for the same thing seems awkward.

[L3MON4D3]

IMHO either snip_env gets always merged with the defaults or we expose the defaults so the users do the merge themselves. Adding two parameters for the same thing seems awkward

Two params for one setting does seem awkward.. Let's just expose the defaults, I can see that always merging is more comfortable, but having some random globals (only for those files) one can't get rid of sounds like trouble

[L3MON4D3]

I feel like we should definitely do something here to make modifying snip_env more comfortable. Reconsidering my earlier opinions, doing a separate config-option which changes the behaviour of snip_env sounds pretty good, actually. Better than doing some weird stuff like than potentially having to reconcile two snip_env-tables, in a non-trivial (well, still trivial, but more to explain -> bad, essentially) manner.

I'd like to have an optionsnip_env_behaviour either "extend" or "set", default "set" ofc, for compatibility.

@hiberabyss Would you like to implement this? The config-file has changed a bit, so you'd need to essentially start from scratch. If you don't wanna do that, I'll do it.

[leiserfg]

I think it should just extend (to be more clear, what dict.update does in python) so the defaults are always there (unless the user overrides them intentionally) having extra things in the environment won't make it much slower.

[L3MON4D3]

dict.update ie. vim.tbl_extend("force", defaults, user), right? I'm not worried about performance, more about weird errors due to using locally undefined, but globally available variables (like, for _, smth in ipairs(...) do <use i here> end would not cause a "i is nil"-error, but something more confusing)

But it does seem like the most comfortable option, just drop whatever you need additionally into snip_env... How about snip_env_behaviour, but the default-behaviour is "extend"? That would also be the most usable (no modifications for the common case of extending the default), with the option to "set" the environment if one so desires? (tbh, I want to just set it, that's mainly why I'd like to have that option :D)

[leiserfg]

dict.update ie. vim.tbl_extend("force", defaults, user), right?

Yes, that. I'm against adding several options for that, maybe make it accept a table for extend or a (default)->final function if someone wanna set (or remove).

[L3MON4D3]

dict.update ie. vim.tbl_extend("force", defaults, user), right?

Yes, that. I'm against adding several options for that, maybe make it accept a table for extend or a (default)->final function if someone wanna set (or remove).

Function is also an idea, but phuuuh, that's way to complicated. Maybe always-extend is the best choice. Alternatively: hide the behaviour in the table, something like

snip_env={
  __snip_env_behaviour = "set",
 ...
}

Feel like that should be unobtrusive enough to not cause any issues

[leiserfg]

👍

[L3MON4D3]

Superseded by #765