-
Notifications
You must be signed in to change notification settings - Fork 19
composable requirements #91
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
base: main
Are you sure you want to change the base?
composable requirements #91
Conversation
class NegativeRequirement(Requirement): | ||
def __init__(self, requirement: Requirement,): | ||
self.requirement = requirement | ||
|
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 kind of a weird thing to prompt with. But it's hard to know what a reasonable prompt would be here.
class ConjunctiveRequirement(Requirement): | ||
def __init__(self, requirements: list[Requirement],): | ||
self.requirements = requirements | ||
|
||
@property | ||
def description(self): | ||
return "\n* ".join( | ||
["Satisfy all of these requirements:"] + \ | ||
[r.description for r in self.requirements]) | ||
|
||
def validate(self, *args, **kwargs): | ||
results = [r.validate(*args, **kwargs) for r in self.requirements] | ||
return ValidationResult( | ||
result = all(results), | ||
reason = "\n* ".join( | ||
["These requirements are not satisfied:"]+ | ||
[r.reason for r in results if not r]), | ||
score = max([r.score for r in results if not r])) |
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.
In the rest of Mellea, a list of requirements is implicitly conjunctive. So does it make sense to have a ConjunctiveRequirement
that behaves differently from a list of reuqirements? Or should we explicitly not have ConjunctiveRequirement
s because the correct way of doing this is to pass both into the requirements=
kwarg?
@property | ||
def description(self): |
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.
description
is NOT a @property
on the base class or in other Requirement types. Maybe it should be, but we should do this the same everywhere. Using property here but not in the other Requirement classes seems like a recipe for confusion down the road.
There are failing quality checks. Please use the pre-commit hooks. Assuming you have already created a venv and installed Mellea (editable), you can install the hooks by running the following commands in the root of your mellea checkout: uv pip install -e . --group dev && pre-commit install Once installed, you can still commit over errors using the |
work in progress sketch for discussion
#22