Repository navigation
Refactor edge names - #38
Conversation
| ports: list[ElkPort] = [] | ||
|
|
||
| for index, position in enumerate(positions): | ||
| for index, position in enumerate(input_positions): |
There was a problem hiding this comment.
For this refactoring, it was easier to _convert_positions_to_elk_ports to remove the need for a id_prefix.
Personally, I don't mind the added duplication since I find the code more readable like this. But I can refactor further if you wish.
There was a problem hiding this comment.
I'm also in favor of the duplication. Makes the input/target and output/source semantic explicit.
79486c2 to
f0f7f6f
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
| ports: list[ElkPort] = [] | ||
|
|
||
| for index, position in enumerate(positions): | ||
| for index, position in enumerate(input_positions): |
There was a problem hiding this comment.
I'm also in favor of the duplication. Makes the input/target and output/source semantic explicit.
|
|
||
|
|
||
| def get_task_id_from_target_id(target_id: str) -> str: | ||
| return target_id.split(".input.")[0] |
There was a problem hiding this comment.
If for a funky reason we have a task ID named : "pipeline.output.normalize" we build:
"pipeline.output.normalize.output.result".
get_task_id_from_source_id will return the wrong task id : "pipeline".
Maybe better to perform right split :
def get_task_id_from_source_id(source_id: str) -> str:
return source_id.rsplit(".output.", 1)[0]There was a problem hiding this comment.
Actually, you have the same problem if the input is named something like input.value or similar.
I'll implement the suggestion but I think we should rethink this part later.
bda8756 to
80bed6b
Compare
80bed6b to
862b389
Compare
2 participants
PR summary
Fix #35 (comment)
I extract the edge naming into util functions, removing the implicit coupling between the creation of ports and of edges.
I also add util functions at the same place to extract the
task_idas mentioned in my previous comment. The coupling is still implicit but at least creation and extraction live at the same place.I didn't go further since I think the refactoring of the layouting will bring opportunities for more elegant refactoring.
AI Disclosure