-
Notifications
You must be signed in to change notification settings - Fork 309
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
Core & Internals: core/rule.py sqla20 syntax #6057 #6493
Core & Internals: core/rule.py sqla20 syntax #6057 #6493
Conversation
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.
Looks promising! Most of my comments are of an aesthetic nature, but some are genuine issues and I’m concerned that our tests didn’t pick up on them.
ca4e11c
to
6791dff
Compare
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.
This is shaping up nicely. Following my second full review, I am now more confident that the new queries are correct.
8b77972
to
79db18d
Compare
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 would have liked a third full review, but I think it has been more than enough.
PR submitted for only rule.py as this file contains ~1000 additions/deletions.