-
Notifications
You must be signed in to change notification settings - Fork 42
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
add support for the arity setting for logical operators in the filter builder #339
base: master
Are you sure you want to change the base?
Conversation
...in the filter builder
for simpler reading code and that all filters are in one place, and not added separately in other locations of the code
The only thing I'm not sure that I changed the code everywhere. See, please, so that there are no extra lines that are no longer needed at this implementation. For example, these lines are confused me: Lines 335 to 337 in 30c39e1
and Lines 367 to 369 in 30c39e1
It seems to me that since the operators are added to the filter_util.filters table, it is no longer necessary to add separately in suggestions table, but I'm not sure about it.
|
oops, sorry, I forgot to delete my localization code |
tabs/search/frame.lua
Outdated
@@ -422,7 +422,7 @@ do | |||
add_form_component() | |||
end | |||
end | |||
input:SetOptions({'and', 'or', 'not', unpack(aux.keys(filter_util.filters))}) | |||
input:SetOptions({'not', unpack(aux.keys(filter_util.filters))}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the reason I added them here is so that they appear first in the suggestions (since the filter_util filters are in a map which will return them in some random order) so ideally that should be preserved somehow.
Also, arity is actually already supported, not sure if you're aware of this, it's not very elegant but you can enter for example and3 as an operator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the reason I added them here is so that they appear first in the suggestions (since the filter_util filters are in a map which will return them in some random order) so ideally that should be preserved somehow.
Yes, you are right about the display order. I came up with how to save the order of displaying filters and even set and change it at any desired moment. I will make a commit a little later.
Also, arity is actually already supported, not sure if you're aware of this, it's not very elegant but you can enter for example and3 as an operator.
No, I did not know about it. This is now undocumented and not entirely obvious. In ReadMe to the addon, I saw the use of the arity and to use it at first generated a request without arity and then add an arity in the search bar in the desired locations. You do not like my implementation? It seems to me this is the best solution. If an additional editbox is displayed - it is immediately obvious that you can write something there. It is only necessary to actualize a little ReadMe to the addon by adding a description of this feature.
['and'] = { | ||
input_type = 'number', | ||
validator = function(arity) | ||
return arity and arity > 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about the default (infinite) arity?
I guess it could be made optional for that, like "not arity or arity > 0".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understood correctly and under the default (infinite) arity, you mean the case when the arity is not specified, then everything works correctly with the implementation of arity and arity > 0
.
The only thing I don't like is that it is impossible to indicate as an arity value "1" (which is equivalent when the arity is not specified). I do not know how to solve this problem, but I think this is not a significant problem.
Done :-) |
82e3148
to
0259b2a
Compare
https://youtu.be/4C5JOkfY-mA