-
Notifications
You must be signed in to change notification settings - Fork 3
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
Allow measures to specify dummy data additional population constraint #2328
Conversation
8d02182
to
8fb066d
Compare
Deploying databuilder-docs with Cloudflare Pages
|
90e2baf
to
3059f5a
Compare
ehrql/query_language.py
Outdated
else: | ||
self.additional_population_constraint = None |
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 we need this branch? (the other branch tested for is not None
, rather than e.g. for truthy)
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.
Well, I just blindly copied it from where it was before, but I think you're right, it'll always be None already here
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.
Thanks for doing this!
ehrql/query_language.py
Outdated
self.additional_population_constraint = ( | ||
self.additional_population_constraint._qm_node | ||
) |
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 don't think this is quite right because now the "validate" method is changing the value of self.additional_population_constraint
. So, for example, it will fail if you call it twice in a row.
I think you want to make it a set_additional_population_constraint()
method which takes the value, validates it, and then updates the attribute if it's valid.
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.
Huh that's how I did it first. Will change it back, hadn't considered calling it twice
So it can be shared by dataset and measures dummy data
3059f5a
to
8bf2037
Compare
No description provided.