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

STYLE: fix violation of the DRY principle #154

Merged
merged 1 commit into from
Dec 10, 2024

Conversation

drveles
Copy link
Contributor

@drveles drveles commented Dec 10, 2024

What was changed

The code has been updated in accordance with the DRY principle.

Why?

It is much smaller, more concise and faster to read.

  1. Closes

  2. How was this tested:
    Standard tests must pass.

  3. Any docs updates needed?
    No. The logic has not been changed.

@CLAassistant
Copy link

CLAassistant commented Dec 10, 2024

CLA assistant check
All committers have signed the CLA.

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.

Sometimes in samples we intentionally copy code as a demonstration of what a user might do if they had different code for each conditional. For example, if we really wanted to properly reuse code, we wouldn't have 4 separately named activities, we'd have one order_fruit activity that accepted a fruit type.

But I am ok with this. @dandavison - opinion?

@dandavison
Copy link
Contributor

Nice cleanup @drveles. I agree -- while the activity calls are all the same it's appropriate for them to be written without repetition.

@@ -59,39 +59,22 @@ async def run(self, list: ShoppingList) -> str:
ordered: List[str] = []
for item in list.items:
Copy link
Contributor

Choose a reason for hiding this comment

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

A tiny thing, not related to this PR, but we should avoid using list as a variable name in Python.

@dandavison dandavison merged commit 3f687e4 into temporalio:main Dec 10, 2024
9 checks passed
@drveles drveles deleted the fix-dry-violation branch December 10, 2024 15:24
@drveles
Copy link
Contributor Author

drveles commented Dec 10, 2024

Thank you.
I will probably correct the code according to the remark about the `list'.

@drveles drveles mentioned this pull request Dec 11, 2024
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