Go over the predicate implementations #239
Conversation
As described in hazelcast#233, our predicate implementations had some problems. This is an attempt to solve all of these. The changes done are listed below. - It is moved from `hazelcast.serialization.predicate` to `hazelcast.predicate` package. - `Predicate` and `PagingPredicate` interfaces are properly defined and documented. - All implementations are made protected. - Factory-like functions are provided for each predicate implementation with proper documentatiın instead of aliases. Also, some explicit names are shortened. - `is_equal_to` -> `equal` - `is_not_equal_to` -> `not_equal` - `is_instance_of` -> `instance_of` - `is_like` -> `like` - `is_ilike` -> `ilike` - `is_greater_than` -> `greater_than` - `is_greater_than_or_equal_to` -> `greater_equal` - `is_less_than` -> `less_than` - `is_less_than_or_equal_to` -> `less_equal` - `is_between` -> `between` - `is_in` -> `in_` - `is_not` -> `not_` - `matches_regex` -> `regex`
README.rst
Outdated
| - ``ilike``: Checks if the result of an expression matches some | ||
| string pattern in a case-insensitive manner. | ||
| - ``is_greater_than``: Checks if the result of an expression is greater | ||
| - ``greater_than``: Checks if the result of an expression is greater |
There was a problem hiding this comment.
nit: in other renames _than part was dropped, so may be greater is a better name. The same suggestion applies to less_than.
There was a problem hiding this comment.
Good idea, I changed it
README.rst
Outdated
| - ``greater_than``: Checks if the result of an expression is greater | ||
| than a certain value. | ||
| - ``is_greater_than_or_equal_to``: Checks if the result of an | ||
| - ``greater_equal``: Checks if the result of an |
There was a problem hiding this comment.
nit: to my taste greater_or_equal sounds a bit better. The same suggestion applies to other similar predicates.
There was a problem hiding this comment.
Good idea, this way it will also resemble or_(greater(...), equal(...)) kind of usage more. I changed it
| - ``between``: Checks if the result of an expression is between two | ||
| values (this is inclusive). | ||
| - ``is_in``: Checks if the result of an expression is an element of a | ||
| - ``in_``: Checks if the result of an expression is an element of a |
There was a problem hiding this comment.
nit: does it make sense to rename this and other predicates with _ suffix slightly more verbose, but get rid of the suffix? Or it's a common practice in Python to deal with such suffixes to avoid collisions with reserved keywords?
Alternatively, we could go one step further and expose only a single factory object instead of individual functions. This way suffixes will no longer be required, as functions will turn into methods.
There was a problem hiding this comment.
Do you mean something like this?
class _Predicates:
def and(self, ...):
pass
Predicates = _Predicates() # import and use thisThis is not a valid Python code. I might also misunderstand your suggestion. Could you give a sample code?
For the other question, yes it is pretty common practice to that. For example, take a look at the following link, https://docs.sqlalchemy.org/en/13/core/sqlelement.html, it is a pretty popular Python package
There was a problem hiding this comment.
Yes, I mean something similar to what we have in Node.js client: https://github.com/hazelcast/hazelcast-nodejs-client/blob/master/code_samples/paging_predicate.js#L75
Not sure if that's a good idea to do the same in Python.
There was a problem hiding this comment.
You can already use this version like that
from hazelcast import predicate
p = predicate.equal("a", 2)But, I believe imported functions are easier to use(require less typing and more readable IMHO)
There was a problem hiding this comment.
With TypeScript, it's more convenient to use a single object. But in case of Python it may be another story, so I get the point of keeping things as is.
According to review comments, the followings are renamed. - `greater_than` -> `greater` - `greater_equal` -> `greter_or_equal` - `less_than` -> `less` - `less_equal` -> `less_or_eqaul`
|
Thanks a lot for the review, I am merging this |
2 participants
As described in #233, our predicate implementations had some
problems. This is an attempt to solve all of these.
The changes done are listed below.
hazelcast.serialization.predicatetohazelcast.predicatepackage.PredicateandPagingPredicateinterfaces are properlydefined and documented.
with proper documentatiın instead of aliases.
Also, some explicit names are shortened.
is_equal_to->equalis_not_equal_to->not_equalis_instance_of->instance_ofis_like->likeis_ilike->ilikeis_greater_than->greater_thanis_greater_than_or_equal_to->greater_equalis_less_than->less_thanis_less_than_or_equal_to->less_equalis_between->betweenis_in->in_is_not->not_matches_regex->regex