-
Notifications
You must be signed in to change notification settings - Fork 25
feat: drop regex support for links in metamodel #598
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| # Claude Code creates .claude directory, e.g. for worktrees. | ||
| # As it contains multiple copies of the entire repository, this totally tripps bazel, | ||
| # which then tries to build all the files in there. | ||
| .claude |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,7 +19,6 @@ | |
| default_options, | ||
| local_check, | ||
| ) | ||
| from score_metamodel.metamodel_types import AllowedLinksType | ||
| from sphinx.application import Sphinx | ||
| from sphinx_needs.need_item import NeedItem | ||
|
|
||
|
|
@@ -134,19 +133,23 @@ | |
| # ) | ||
|
|
||
|
|
||
| def _to_link_pattern(value: "str | ScoreNeedType") -> str: | ||
| """Convert a link constraint to a regex pattern. | ||
| def _to_link_pattern(value: ScoreNeedType) -> str: | ||
| """ | ||
| Convert a link constraint to a regex pattern. | ||
|
|
||
| - A plain type name like ``"stkh_req"`` becomes ``^stkh_req__`` | ||
| (matching IDs that start with the type name followed by ``__``). | ||
| - A string already starting with ``^`` is treated as an explicit regex | ||
| and returned unchanged. | ||
| - A ScoreNeedType dict uses its ``mandatory_options.id`` pattern. | ||
| Note: the pattern is already stored in "id" attribute, this is just a helper | ||
| function to retrieve it safely. | ||
| """ | ||
| if isinstance(value, str): | ||
| if value.startswith("^"): | ||
| return value | ||
| return f"^{value}__" | ||
| assert isinstance(value, dict), f"Expected dict for ScoreNeedType, got {value}" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you not do
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, isinstance does not work with TypedDict. As the type info is not a real type AFAIK. |
||
| assert "mandatory_options" in value, ( | ||
| f"ScoreNeedType dict must have 'mandatory_options', got {value}" | ||
| ) | ||
| assert isinstance(value["mandatory_options"], dict), ( | ||
| f"'mandatory_options' must be a dict, got {value['mandatory_options']}" | ||
| ) | ||
| assert "id" in value["mandatory_options"], ( | ||
| f"'mandatory_options' must contain 'id', got {value['mandatory_options']}" | ||
| ) | ||
| return value["mandatory_options"]["id"] | ||
|
|
||
|
|
||
|
|
@@ -160,10 +163,12 @@ | |
| """ | ||
|
|
||
| def _validate( | ||
| attributes_to_allowed_values: AllowedLinksType, | ||
| attributes_to_allowed_values: dict[str, list[ScoreNeedType]] | None, | ||
| mandatory: bool, | ||
| treat_as_info: bool = False, | ||
| ): | ||
| assert attributes_to_allowed_values is not None | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Kinda an antipattern no?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah... its not pretty. This avoid checking the argument on all call sites, which would be even uglier. |
||
|
|
||
| for attribute, allowed_values in attributes_to_allowed_values.items(): | ||
| values = _get_normalized(need, attribute) | ||
| if mandatory and not values: | ||
|
|
@@ -238,7 +243,8 @@ | |
| "mandatory_links", | ||
| "optional_links", | ||
| ): | ||
| assert need_options[o] is not None | ||
| allowed_options.update(need_options[o].keys()) | ||
|
|
||
| extra_options = [ | ||
| option | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,10 +21,15 @@ | |
| graph_check, | ||
| ) | ||
| from sphinx.application import Sphinx | ||
| from sphinx_needs import logging | ||
| from sphinx_needs.config import NeedType | ||
| from sphinx_needs.data import NeedsView | ||
| from sphinx_needs.need_item import NeedItem | ||
|
|
||
| # This is the normal logger for this module, not for warnings on specific needs. | ||
| # Use CheckLogger for that, which allows us to log the need id and location together with the warning message. | ||
| logger = logging.get_logger(__name__) | ||
|
|
||
|
|
||
| def eval_need_check(need: NeedItem, check: str, log: CheckLogger) -> bool: | ||
| """ | ||
|
|
@@ -40,6 +45,7 @@ def eval_need_check(need: NeedItem, check: str, log: CheckLogger) -> bool: | |
| "<": operator.lt, | ||
| ">=": operator.ge, | ||
| "<=": operator.le, | ||
| "contains": lambda a, b: b in a if isinstance(a, str) else False, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This then allows for b to be a non string type. Is that wanted behaviour?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no, but I did not figure out how to say "else fail horribly" - without changing how the function is designed. |
||
| } | ||
|
|
||
| parts = check.split(" ") | ||
|
|
@@ -69,11 +75,9 @@ def eval_need_condition( | |
| Recursively call the eval_need_function for each check and combine the | ||
| results with the binary operation which was specified in the yaml file. | ||
| """ | ||
|
|
||
| oper: dict[str, Any] = { | ||
| "and": operator.and_, | ||
| "or": operator.or_, | ||
| "not": lambda x: not x, | ||
| "xor": operator.xor, | ||
| } | ||
|
|
||
|
|
@@ -91,13 +95,18 @@ def eval_need_condition( | |
| if cond == "not": | ||
| if not isinstance(vals, list) or len(vals) != 1: | ||
| raise ValueError("Operator 'not' requires exactly one operand.") | ||
| return oper["not"](eval_need_condition(need, vals[0], log)) | ||
|
|
||
| if cond in ["and", "or", "xor"]: | ||
| return not eval_need_condition(need, vals[0], log) | ||
|
|
||
| if cond in oper: | ||
| if not isinstance(vals, list) or len(vals) <= 1: | ||
| raise ValueError(f"Operator '{cond}' requires at least two operands.") | ||
|
|
||
| return reduce( | ||
| lambda a, b: oper[cond](a, b), | ||
| (eval_need_condition(need, val, log) for val in vals), | ||
| ) | ||
|
|
||
| raise ValueError(f"Unsupported condition operator: {cond}") | ||
|
|
||
|
|
||
|
|
@@ -175,9 +184,15 @@ def check_metamodel_graph( | |
| "Explanations are mandatory for graph checks." | ||
| ) | ||
| # Get all needs matching the selection criteria | ||
| selected_needs = filter_needs_by_criteria( | ||
| app.config.needs_types, needs_local, needs_selection_criteria, log | ||
| ) | ||
| try: | ||
| selected_needs = filter_needs_by_criteria( | ||
| app.config.needs_types, needs_local, needs_selection_criteria, log | ||
| ) | ||
| except ValueError as e: | ||
| # Turn a 3 page callstack into a readable error message for the user, since | ||
| # this is a configuration error in the yaml file. | ||
| logger.error(f"Error in graph check `{check_name}`: {e}") | ||
| continue | ||
|
|
||
| for need in selected_needs: | ||
| for parent_relation in list(check_to_perform.keys()): | ||
|
|
||
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.
Is this all_need_types? It reads like a filtered version already.
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.
it's "all our types" aka "all types from metamodel"