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

Update-with-start: shopping cart #156

Merged
merged 15 commits into from
Jan 16, 2025
Merged

Update-with-start: shopping cart #156

merged 15 commits into from
Jan 16, 2025

Conversation

dandavison
Copy link
Contributor

@dandavison dandavison commented Dec 12, 2024

  • The Workflow represents a Shopping Cart in an e-commerce application, and
    update-with-start is used to add items to the cart, receiving back the updated cart subtotal.

  • Cannot be merged until UwS is released


@workflow.update
async def add_item(self, item: ShoppingCartItem) -> str:
price = await get_price(item)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you want this to be an activity invocation

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! So (a) too much Typescript, and (b) this is a good example of a mistake that an AI linter could catch but traditional analysis wouldn't.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@dandavison dandavison force-pushed the uws-shopping-cart branch 2 times, most recently from a538c41 to 588f4f1 Compare January 9, 2025 21:21
@dandavison
Copy link
Contributor Author

@cretz @yuandrew thanks for your reviews -- this should be ready now.

Copy link
Member

@cretz cretz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing blocking

Comment on lines 44 to 45
except WorkflowUpdateFailedError:
price = None
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May want a bit more of a conditional to confirm it's the failure you expect instead of swallowing every update failure ever. In .NET I checked the application failure's error type: https://github.com/temporalio/samples-dotnet/blob/86a49887aaa668beb3cadb4fa862574f3295fcc6/src/UpdateWithStartLazyInit/Program.cs#L85-L90

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call, done.

@@ -0,0 +1 @@
TASK_QUEUE = "uws"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the task queue should be named specific to the sample so it doesn't clash with others (also, this constant isn't used in starter.py as might be expected)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@dandavison dandavison merged commit 5541408 into main Jan 16, 2025
10 checks passed
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.

3 participants