-
Notifications
You must be signed in to change notification settings - Fork 79
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
Wrap the Filter interface #2336
Conversation
return Filter(j_filter=_JFilterAnd.of(*[f.j_filter for f in filters])) | ||
|
||
|
||
def not_(filter_: Filter) -> Filter: |
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.
Is this more pythonic, or is my_filter.not()
? The java interface has not()
as part of the interface.
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.
'not' is a Python keyword, can't be used as an identifier
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.
We could do my_filter.not_()
(we wouldn't need to provide it in java since we can do it in the wrapper) if desired.
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.
Or, as property: my_filter.not_
raise DHError(e, "failed to create filters.") from e | ||
|
||
|
||
def or_(filters: List[Filter]) -> Filter: |
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.
Looking at this, and unit tests, it looks like we have:
my_table.where(or_(Filter.from_(["a > 100", "b < 1000", "c < 0"])))
I'm wondering if we don't want something like:
my_table.where(or_(["a > 100", "b < 1000", "c < 0"]))
And, it might not just be strings, but filters themselves:
f1 = and_(["a > 100", "b <1000"])
f2 = and_(["c > 1", "d < 1"])
f = or_([f1, f2, "z>42"])
my_table.where(f)
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.
Not sure we want to allow mixing Filter objects with conditional strings. Arguably it might be more confusing than helpful.
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'm fine w/ the conclusion as long as we are happy.
f1 = and_(Filter.from_(["a > 100", "b <1000"]))
f2 = and_(Filter.from_(["c > 1", "d < 1"]))
f = or_([f1, f2, Filter.from_("z>42")])
my_table.where(f)
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'm "ok" w/ as-is; unless we think there are usability concerns wrt creating filters.
Fixes #2335