[feature] implement condition objects

[atticus-sullivan]

Refer to #600 for most details. As it turned out that memoization is pretty hard to achive (moreover the invalidation) and we're not sure if it is worth it, we decided to do the basic condition objects (and -> * , or -> +) in a separate PR.

TODOs:

• Write some tests for this
• Write documentation
• Move to extras/conditions/ / split into _expand/_show and _objects

Anything I'm missing?

[L3MON4D3]

Nice, I think that's it, not missing anything 👍

[atticus-sullivan]

Lua 5.4 reference states that for __eq

The result of the call is always converted to a boolean.

The for luajit more relevant Lua 5.1 reference contains no such statement (lua5.2 states Note that the result is always a boolean.).

In my tests this also holds. Lua seems to be doing something like return self.__eq(other) ~= nil and self.__eq(other) ~= false.
As we need to return a function from the operator, this is a problem for us.

In effect we cannot use ==, < and >. Operators which are left are

• __sub (- bad Idea as we're using the __unm for not)
• __div (/)
• __mod (%)
• __concat (..)
• __len (#)

Nothing seems to be really fitting for the xnor aka == operator. Any ideas?

[atticus-sullivan]

Should we add a composite test of multiple operators (including parentheses)? And if so any suggestions what expression might be interesting?

[bew]

Also in the spec, when you require stuff from luasnip.extras.expand_conditions, the local var is currently always called cond, I think it'd be more readable to have:

• mkcond when importing make_condition
• conds for the module luasnip.extras.expand_conditions

[L3MON4D3]

Lua 5.4 reference states that for __eq

The result of the call is always converted to a boolean.

The for luajit more relevant Lua 5.1 reference contains no such statement (lua5.2 states Note that the result is always a boolean.).

In my tests this also holds. Lua seems to be doing something like return self.__eq(other) ~= nil and self.__eq(other) ~= false. As we need to return a function from the operator, this is a problem for us.

In effect we cannot use ==, < and >. Operators which are left are

• __sub (- bad Idea as we're using the __unm for not)
• __div (/)
• __mod (%)
• __concat (..)
• __len (#)

Nothing seems to be really fitting for the xnor aka == operator. Any ideas?

Oh wow, that's unfortunate :/
Tough, I'm not sure whether a confusing symbol is better than not having one.. == is also quickly achieved via -(a^b), but having a direct operator for it would be better.
Of the above, I think % would be best. It feels the most unusual and hopefully triggers a "Okay no idea what this is supposed to mean, let's have a look at the docs", instead of an "Ahh, this probably means ___, alright"

Also in the spec, when you require stuff from luasnip.extras.expand_conditions, the local var is currently always called cond, I think it'd be more readable to have:

• mkcond when importing make_condition
• conds for the module luasnip.extras.expand_conditions

Ah, expand_conditions used to be named conditions, but when show_conditions was introduced, this lead to some ambiguity, and was changed.
Putting the condition-objects into their own module sounds good though👍

One important fact: show_conditions can be passed to expand_condition, so we could have

• extras/show_conditions (which contains the conditions we currently have (they only rely on line_to_cursor))
• extras/expand_condition, which re-exports all show_conditions (passing a show_condition to (expand_)condition seems fishy, and the separation could prevent a breaking change if show_conditions have to be incompatible with expand_condition for whatever reason)
• extras/condition (not sure about that name tbh, condition_objects is an alternative), which is relied on by both modules, and implements the boolean expression-functionality

Would be a bit nicer if all were in one common directory, so extras/conditions/{init,show,expand}, but that would be a breaking change (although, I'm not completely averse to making that one, just have to release 1.1.0 before going straight to 2.0.0 xD)

[L3MON4D3]

Should we add a composite test of multiple operators (including parentheses)? And if so any suggestions what expression might be interesting?

Ah tests, right: One fun way to test them would be running the same expression for all true/false combinations of their variables, and comparing that to the expected results (if it gets too complex, don't do it though :D)
But I don't have any specific expressions in mind, just testing every operator (and maybe composability (negation is quickly implement if you end up using the truth-tables ;) )) once should do it.

[atticus-sullivan]

Ah tests, right: One fun way to test them would be running the same expression for all true/false combinations of their variables, and comparing that to the expected results (if it gets too complex, don't do it though :D)

That's what I'm currently doing for the single operators.

But I don't have any specific expressions in mind, just testing every operator (and maybe composability (negation is quickly implement if you end up using the truth-tables ;) )) once should do it.

Hm then I'll have to think of an expression.

Well just thought of composite ones and there is work to do (currently these opterators simply return functions -> e.g, (f1+f2)*(f3+f4) shouldn't work (* not defined for functions) so I'll have to wrap those in tables as well)
[always good to have (or at least think about) tests xD]

[atticus-sullivan]

Tough, I'm not sure whether a confusing symbol is better than not having one.. == is also quickly achieved via -(a^b), but having a direct operator for it would be better.
Of the above, I think % would be best. It feels the most unusual and hopefully triggers a "Okay no idea what this is supposed to mean, let's have a look at the docs", instead of an "Ahh, this probably means ___, alright"

I agree. But I'm not sure as well whether not implement == or use % (well no one foces one to use % though 🤔)

[atticus-sullivan]

Would be a bit nicer if all were in one common directory, so extras/conditions/{init,show,expand}, but that would be a breaking change

I'd be in favor of this. Whether we do this or not is your call though.

[L3MON4D3]

That's what I'm currently doing for the single operators.

Hah, Perfect :D

Well just thought of composite ones and there is work to do (currently these opterators simply return functions -> e.g, (f1+f2)*(f3+f4) shouldn't work (* not defined for functions) so I'll have to wrap those in tables as well) [always good to have (or at least think about) tests xD]

Ohh wow, yeah completely missed that as well 🤦

I agree. But I'm not sure as well whether not implement == or use % (well no one foces one to use % though thinking)

Only downside I can see is confusion as to what % does, but that's better than not offering it at all, sooo let's include it

I'd be in favor of this. Whether we do this or not is your call though.

Yeah let's do it backwards-compatibly, re-export extras/conditions/expand (not 100% happy with that name btw, any ideas?) in extras/expand_conditions for now.

[atticus-sullivan]

Yeah let's do it backwards-compatibly

Done.

not 100% happy with that name btw, any ideas?

What name exactly are you not happy with, extras/condition or extras/condition/expand?

[L3MON4D3]

What name exactly are you not happy with, extras/condition or extras/condition/expand?

Oh, just the expand (one more idea: for_expand. It's more descriptive, but still not perfect)

[atticus-sullivan]

Hm, I've got no really good idea as well.

[L3MON4D3]

Okay, I think this is actually it, as far as functionality is concerned :D

[atticus-sullivan]

So just to summarize what is to be done until the merge:

• write documentation
• rename expand.lua/show.lua modules if we find a better name (in my opinion conditions.expand looks better than conditions.for_expand, but maybe we find something better (though no reason to delay the merge in the end if we don't come up with something better, I think))
• update line_end (compare strings instead of lengths)
• rebase -> merge

(just summarizing as I can't do it right now (will be tomorrow), and we don't forget something)

[atticus-sullivan]

Hm, no Action run which rebuilds the documentation 🤔, well will come back with the next pushes I guess.
So any comments before I squash/rebase?

[L3MON4D3]

Ah, also: would you include a deprecation-notice for expand_conditions in the first commit? (Or, optionally, extract the split of expand_conditions into a separate commit (before/after condition-objects wouldn't make much of a difference afaict))

[atticus-sullivan]

Phu I know no way how to cleanly split up the commit afterwards (would be interested though if you know one). So I'll just add the note in the commit message.
By deprecation-notice you mean a comment in the code or a note in the commit message?

[L3MON4D3]

Phu I know no way how to cleanly split up the commit afterwards (would be interested though if you know one). So I'll just add the note in the commit message. By deprecation-notice you mean a comment in the code or a note in the commit message?

Just the commit message 👍
AFAIK packer highlights commits that contain an exclamation mark, so best format it as feat! or refactor!, something like that.

Concerning the split yeeeah, that would be a bit involved, I think

• soft-reset all
• commit conditions/init.lua (+tests??)
• commit line-ending-change
• commit changes expand_conditions, conditions/expand and conditions/show (+tests)
• commit doc

But that's honestly too much work for the tiny benefit of having the commits techincally more "pure" (really a technicality, they're all merged at once anyway)

[atticus-sullivan]

AFAIK packer highlights commits that contain an exclamation mark, so best format it as feat! or refactor!, something like that.

Is this what you had in mind?

[L3MON4D3]

Yup, that's it 👍👍
Thank you for another clean addition!!

[mathjiajia]

It seems you haven't updated the default config file

LuaSnip/lua/luasnip/config.lua

Line 139 in aa7acef

conds = require("luasnip.extras.expand_conditions"),

[atticus-sullivan]

Oh yes. Didn't thought of this file. Just noticed that the tests also still use the now deprecated module

[L3MON4D3]

Just noticed that the tests also still use the now deprecated module

I think that's okay, I don't dislike having those as security that the functions can still be accessed via the deprecated way. About config.. we should change that some time, but there's no hurry