-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
[13.0] Create module base_domain_inverse_function #2474
[13.0] Create module base_domain_inverse_function #2474
Conversation
216a47e
to
76b6e61
Compare
to fix the build you need to change from gitlab to github server-tools/.pre-commit-config.yaml Line 66 in ef4e864
|
@i-vyshnevska that probably won't be enough, we need to move to GH actions but there are some issues in this repo, cf #2459 |
self.assertEqual( | ||
inverse_OR(self.complex_domain_or_and_and), | ||
result | ||
) |
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.
shouldn't you add a test that ensures that an OR domain isn't split by inverse_AND
, and an AND domain isn't split by inverse_OR
self.assertEqual(inverse_OR(self.complex_domain_and_or_or), self.complex_domain_and_or_or)
self.assertEqual(inverse_AND(self.complex_domain_or_and_and), self.complex_domain_or_and_and)
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.
There's no sense to use the inverse_OR
function on a domain that doesn't begin with an OR operator, as it means such domain wasn't created with OR
function. I could still add a test and raise an error in this case.
) | ||
|
||
|
||
def inverse_combine(domain, 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 find it very difficult to understand.
Domains use infix notation a + b -> + b a
.
I remember in compilation courses at school, we created a lexer for that notation, and I would have gone this way.
The grammar here is quite easy: |, &, ! and tuples
, I think it would be easier to review and to maintain.
However, it might take some time to implement.
Ping me if you have questions.
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.
Feel free to propose another implementation if you can do better 😝
76b6e61
to
ef7d3fc
Compare
ef7d3fc
to
3c8d2a0
Compare
/ocabot merge nobump |
On my way to merge this fine PR! |
This PR has the |
Congratulations, your PR was merged at fa15dcd. Thanks a lot for contributing to OCA. ❤️ |
This module provides functions to decompose normalized domains
into domains operands, as these functions are the inverse of
AND
andOR
functions available inodoo.osv.expression
.