Skip to content

Add description field to Workflow constructor#157

Merged
jefflloyd merged 3 commits intomainfrom
jefflloyd/update-workflow-constructor-with-desc
Apr 24, 2023
Merged

Add description field to Workflow constructor#157
jefflloyd merged 3 commits intomainfrom
jefflloyd/update-workflow-constructor-with-desc

Conversation

@jefflloyd
Copy link
Copy Markdown
Contributor

Description of changes (updated or new workflows)

Adds a description field to the Workflow constructor. Previously it was hardcoded to None, this allows it to be set when calling Workflow::new().

@jefflloyd jefflloyd requested a review from alokedesai April 24, 2023 17:08
Comment thread workflow-types/src/lib.rs Outdated
command: command.into(),
tags: vec![],
description: None,
description,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

thoughts on setting this via a builder pattern instead?

fn with_description(mut self, description: String) -> Self {
}

Passing this through the constructor can be a little awkward if majority of the time it will be just None (in which case it's not clear what the description even is). This also matches the API for setting the arguments

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Totally see that reasoning - will make the change!

@jefflloyd jefflloyd requested a review from alokedesai April 24, 2023 18:12
Copy link
Copy Markdown
Member

@alokedesai alokedesai left a comment

Choose a reason for hiding this comment

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

🚀

Comment thread workflow-types/src/lib.rs
self
}

pub fn with_description(mut self, description: String) -> Self {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: you shouldn't need a clone here

@jefflloyd jefflloyd merged commit 01f4093 into main Apr 24, 2023
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.

2 participants