Add rule stop functionality #106
Conversation
| StartDate = NewType("StartDate", date) | ||
| EndDate = NewType("EndDate", date) | ||
| CohortLabel = NewType("CohortLabel", str) | ||
| RuleStop = NewType("RuleStop", str) |
There was a problem hiding this comment.
Perhaps this should be an Enum?
There was a problem hiding this comment.
Or even a bool with @field_validator and @field_serializer decorators?
There was a problem hiding this comment.
Yeah, I think a bool would be better.
There was a problem hiding this comment.
@acerathereinspirative-nhs-nvir thanks, we just implemented the enum.
Enum looks better in terms of handling when rule_stop is "" or None situations. Also less code than the bool setup
class RuleStop(StrEnum):
YES = "Y"
NO = "N"
@classmethod
def _missing_(cls, value: object) -> RuleStop | None:
if isinstance(value, str) and value == "":
return cls.NO
return None
kindy, is this ok ?
There was a problem hiding this comment.
Isn't None equivalent to False? Can't we default to False if the value isn't "Y"?
There was a problem hiding this comment.
?
It's still an Enum. I think a bool would better fit the domain and behaviour.
There was a problem hiding this comment.
Got it, we will implement it.
might include a integration test as well because the build factory is intelligent and seems to be setting "Y" as true by itself, without going through the "@field_validator"
| and (ir.cohort_label is None or (ir.cohort_label in self.person_cohorts)) | ||
| ] | ||
| exclude_capable_rules = list( | ||
| takewhile( |
There was a problem hiding this comment.
Nice use of itertools. :-)
There was a problem hiding this comment.
Unfortunately i implemented it wrongly. We are pushing the fix :)
acerathereinspirative-nhs-nvir
left a comment
acerathereinspirative-nhs-nvir
left a comment
There was a problem hiding this comment.
Looks good now.
tests/integration/conftest.py
Outdated
| iterations=[ | ||
| rule.IterationFactory.build( | ||
| iteration_rules=[rule.PersonAgeSuppressionRuleFactory.build()], | ||
| iteration_rules=[ |
There was a problem hiding this comment.
Argh, hang on - why are we setting all this in the conftest.py? Do we need these values for all tests?
There was a problem hiding this comment.
Can we discuss?
There was a problem hiding this comment.
sorry , the code was not complete(need modifications). we have to parameterise, so will not be editing the conftest however. Should have added TODO. also making it to draft until its complete
tests/integration/conftest.py
Outdated
| iterations=[ | ||
| rule.IterationFactory.build( | ||
| iteration_rules=[rule.PersonAgeSuppressionRuleFactory.build()], | ||
| iteration_rules=[ |
There was a problem hiding this comment.
Can we discuss?
Karthikeyannhs
force-pushed
the
feature/eli-221-add-rule-stop-functionality
branch
from
f07ddc9 to
1dbf91a
Compare
Karthikeyannhs
force-pushed
the
feature/eli-221-add-rule-stop-functionality
branch
from
13357ad to
27a3ce0
Compare
acerathereinspirative-nhs-nvir
left a comment
acerathereinspirative-nhs-nvir
left a comment
There was a problem hiding this comment.
Happy now the conftest is back as it was.
2 participants
Description
Context
Type of changes
Checklist
Sensitive Information Declaration
To ensure the utmost confidentiality and protect your and others privacy, we kindly ask you to NOT including PII (Personal Identifiable Information) / PID (Personal Identifiable Data) or any other sensitive data in this PR (Pull Request) and the codebase changes. We will remove any PR that do contain any sensitive information. We really appreciate your cooperation in this matter.