-
Notifications
You must be signed in to change notification settings - Fork 11
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
feat(sdk-python): Port in-line expressions to Python SDK #1166
Conversation
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.
I have approve it, but before merging check if it is possible to get rid of the dependency at WfRunVariable and use the WorkflowCondition.
The other WfRunVariable parameter are attributes of the class, WorkflowThread would be a dependency that maybe we can avoid, if we can the better
sdk-python/littlehorse/workflow.py
Outdated
|
||
return self | ||
|
||
def __set_default(self, default_value: Any) -> None: |
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.
1 underscore for internal 2 for language related ones.
example __init__
2 underscore
|
||
|
||
class WfRunVariable: | ||
def __init__( | ||
self, | ||
variable_name: str, | ||
variable_type: VariableType, | ||
parent: WorkflowThread, |
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.
if the default value is None use = None
here
Optional[WorkflowThread]
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.
I'm not totally convince if we should do this.
Actually we can do something like:
def is_equal_to(self, rhs: Any) -> WorkflowCondition:
return WorkflowCondition(self, Comparator.EQUALS, rhs)
Even though we can implement it similar to java, I would prefer to reduce the dependencies in python.
Circular dependencies errors are very common in python. They (python devs) are going to get rid of it, and make it a more java like language, but i do not know when.
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.
@sauljabin I agree with your proposal, it is unnecessary to depend on the parent condition methods when creating a new WorkflowCondition is so simple. If the code for creating a new WorkflowCondition were more complex, it may make sense to centralize it in one place and rely on this dependency injection or another class to hold the methods.
There is one additional method that may not make sense to port without injecting the parent thread dependency:
@Override
public void assignTo(Serializable rhs) {
parent.mutate(this, VariableMutationType.ASSIGN, rhs);
}
Without injecting the parent WorkflowThread at the constructor, this method may not make sense or be easy to use. Instead, we would have to pass it to the method as an argument, which would not save the user much code and (in my opinion) be less readable.
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.
You are right, I did not see the mutate
…lehorse-enterprises/littlehorse into feat/expressions-python-sdk
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.
I'm confused. Don't you need to edit the to_variable_assignment()
method in order for this to work?
…lehorse-enterprises/littlehorse into feat/expressions-python-sdk
This PR ports the in-line expression changes to the Python SDK.