Skip to content
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

Merged
merged 26 commits into from
Dec 2, 2024

Conversation

Snarr
Copy link
Member

@Snarr Snarr commented Nov 27, 2024

This PR ports the in-line expression changes to the Python SDK.

Copy link
Member

@sauljabin sauljabin left a 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


return self

def __set_default(self, default_value: Any) -> None:
Copy link
Member

@sauljabin sauljabin Nov 27, 2024

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,
Copy link
Member

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]

Copy link
Member

@sauljabin sauljabin Nov 27, 2024

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.

Copy link
Member Author

@Snarr Snarr Nov 27, 2024

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.

Copy link
Member

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

Copy link
Member

@coltmcnealy-lh coltmcnealy-lh left a 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?

sdk-python/littlehorse/workflow.py Outdated Show resolved Hide resolved
sdk-python/littlehorse/workflow.py Show resolved Hide resolved
@Snarr Snarr merged commit 9d1de83 into master Dec 2, 2024
16 checks passed
@Snarr Snarr deleted the feat/expressions-python-sdk branch December 2, 2024 23:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants