-
Notifications
You must be signed in to change notification settings - Fork 0
Feature/SOF-7766-3 Update: methods implementation for MIN NB to run #121
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
Conversation
| It provides common methods for managing units in both Workflow and Subworkflow classes. | ||
| """ | ||
|
|
||
| units: List["Unit"] |
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.
Why is it in quotes?
tests/py/test_workflow.py
Outdated
| from mat3ra.ade.application import Application | ||
| from mat3ra.mode.method import Method | ||
| from mat3ra.mode.model import Model | ||
| from mat3ra.standata.workflows import WorkflowStandata |
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.
Flowchart units managers should be tested
tests/py/test_unit.py
Outdated
| assert data["head"] is True | ||
|
|
||
|
|
||
| def test_add_context_to_relaxation_unit(): |
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.
Too complex of a test
| self._add_to_list(self.units, unit, head, index) | ||
| self.set_units(self.set_next_links(self.set_units_head(self.units))) | ||
|
|
||
| # TODO: Consider removing setNextLinks and setUnitsHead calls when flowchart designer implemented. |
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.
Remove
| - `add_target(target)` - Add target | ||
| - `remove_target(target)` - Remove target | ||
| - `has_feature(feature)` - Check feature exists | ||
| - `has_target(target)` - Check target exists |
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.
Remove the functions that we don't have
| @@ -0,0 +1,87 @@ | |||
| # TODO: We need periodic_table.js equivalent in Python | |||
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.
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 meant to be changed in the task for Context Providers. This is just a comment to implement that
| then: str = Field(default="") | ||
| else_: str = Field(default="", alias="else") | ||
| input: list = Field(default_factory=list) | ||
| maxOccurrences: int = Field(default=1) |
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.
We should get the default values from schema
|
|
||
|
|
||
| class DataFrameIOUnit(IOUnit): | ||
| subtype: str = Subtype.dataFrame |
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.
Remove
| from mat3ra.utils.uuid import get_uuid | ||
|
|
||
|
|
||
| def generate_uuid() -> str: |
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.
Remove
| return get_uuid() | ||
|
|
||
|
|
||
| def find_by_name_or_regex( |
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.
Should be elsewhere
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.
And tested
|
|
||
| @classmethod | ||
| def from_subworkflow(cls, subworkflow: Subworkflow) -> "Workflow": | ||
| raise NotImplementedError |
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.
Let's create a separate branch without all NotImplementedError
No description provided.