feat: drop regex support for links in metamodel#598
Conversation
AlexanderLanin
requested review from
MaximilianSoerenPollak,
dcalavrezo-qorix and
nradakovic
as code owners
License Check Results🚀 The license check job ran with the Bazel command: bazel run --lockfile_mode=error //src:license-checkStatus: Click to expand output |
|
The created documentation from the pull request is available at: docu-html |
MaximilianSoerenPollak
left a comment
MaximilianSoerenPollak
left a comment
There was a problem hiding this comment.
Some questions
| if value.startswith("^"): | ||
| return value | ||
| return f"^{value}__" | ||
| assert isinstance(value, dict), f"Expected dict for ScoreNeedType, got {value}" |
There was a problem hiding this comment.
Can you not do is instance to make sure it's actually a ScoreNeedType?
There was a problem hiding this comment.
No, isinstance does not work with TypedDict. As the type info is not a real type AFAIK.
| mandatory: bool, | ||
| treat_as_info: bool = False, | ||
| ): | ||
| assert attributes_to_allowed_values is not None |
There was a problem hiding this comment.
Kinda an antipattern no?
If the typehint is allowed to be None, but here you assert that it should never happen?
There was a problem hiding this comment.
yeah... its not pretty. This avoid checking the argument on all call sites, which would be even uglier.
| "<": operator.lt, | ||
| ">=": operator.ge, | ||
| "<=": operator.le, | ||
| "contains": lambda a, b: b in a if isinstance(a, str) else False, |
There was a problem hiding this comment.
This then allows for b to be a non string type. Is that wanted behaviour?
There was a problem hiding this comment.
no, but I did not figure out how to say "else fail horribly" - without changing how the function is designed.
|
|
||
| .. std_req:: ASPICE 40 IIC requirement | ||
| :id: std_req__aspice_40__iic_1 | ||
|
|
There was a problem hiding this comment.
Uhh idk if this will work?
As a 1 line seperation might interpret it as content for the need.
Though unsure about that, has been a while.
There was a problem hiding this comment.
doesnt matter, as next line is a comment. Whether its a comment, real content, or need content. dont care.
| need_type["optional_links"], | ||
| # "mandatory_links_str" is used to keep only metamodel sourced types. That key is so | ||
| # specific, that noone but our metamodel should be using it. | ||
| all_need_types = { |
There was a problem hiding this comment.
Is this all_need_types? It reads like a filtered version already.
There was a problem hiding this comment.
it's "all our types" aka "all types from metamodel"
| status: ^(valid|draft)$ | ||
| optional_links: | ||
| complies: std_wp, ^std_req__aspice_40__iic.*$ | ||
| # FIXME: only ^std_req__aspice_40__iic.*$ |
There was a problem hiding this comment.
FIXME Still needed? THis is now implemented with the graph check no?
There was a problem hiding this comment.
changed from fixme to simple comment referring to graph check
|
|
||
| # At parse time these are None, as they need to be filled post-parsing when all types are available to resolve the link targets. | ||
| # The dict maps the link name to a list of need types that are allowed as targets for that link. | ||
| mandatory_links: None | dict[str, list[ScoreNeedType]] |
There was a problem hiding this comment.
Default them to None ? Or is that automatic?
There was a problem hiding this comment.
Unfortunately there is no default in TypedDict. All call sites adapted.
MaximilianSoerenPollak
merged commit 11ab363
into
eclipse-score:main
github-project-automation
Bot
moved this from Backlog
to Done
in INF - Infrastructure Community
2 participants
simplify the code and the metamodel