-
Notifications
You must be signed in to change notification settings - Fork 72
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
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, I changed it
README.rst
Outdated
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, this way it will also resemble or_(greater(...), equal(...))
kind of usage more. I changed it
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean something like this?
class _Predicates:
def and(self, ...):
pass
Predicates = _Predicates() # import and use this
This 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
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.predicate
tohazelcast.predicate
package.Predicate
andPagingPredicate
interfaces are properlydefined and documented.
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