refactor: use difficulty labels and classes, tidy up domain slightly#29
refactor: use difficulty labels and classes, tidy up domain slightly#29exploreriii wants to merge 1 commit intomainfrom
Conversation
Signed-off-by: exploreriii <133720349+exploreriii@users.noreply.github.com>
| ONBOARDING_STAGES = ( | ||
| FIRST_PR, | ||
| MERGED_FIRST_PR, | ||
| ) No newline at end of file |
There was a problem hiding this comment.
The GFI_STARTER is missing.
| normalized = {label.lower() for label in issue_labels} | ||
| return bool(self.labels.intersection(normalized)) | ||
|
|
||
| def __contains__(self, issue_labels: Iterable[str]) -> bool: |
There was a problem hiding this comment.
I would suggest, removing the __contains__ method. the current implementation accepts Iterable[str] but in case a single string is passed (eg: "good first issue") then this will be produced as set(), which could lead to unexpected or incorrect matching behavior.
since we already have
src\hiero_analytics\domain\difficulty.py
if level.label_spec.matches(issue_labels): line: 87the __contains__ is not necessary for this scenario.
though i appreciate a second opinion from @MonaaEid @undefinedIsMyLife
| for level in DIFFICULTY_LEVELS: | ||
| if level.label_spec.matches(issue_labels): | ||
| return level.name | ||
|
|
There was a problem hiding this comment.
Since we are moving toward a domain model, would it be better to return the DifficultyLevel object itself rather than just the string name? This would allow calling scripts to access the .order attribute if they need to sort results later.
Instead of level.name if we can return level
|
|
||
| name: str | ||
| labels: set[str] | ||
| labels: FrozenSet[str] |
There was a problem hiding this comment.
Since matches() relies on lowercase comparison, we should ensure the internal labels set is always lowercase. Instead of just adding a documentation note, we can enforce this in the code using a __post_init__ block. This prevents 'silent bugs' if a future contributor accidentally adds a label with capital letters.
def __post_init__(self):
object.__setattr__(self, 'labels', frozenset(l.lower() for l in self.labels))
What do you think about making the class handle this normalization automatically?
4 participants
Fixes #28