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.

Sign up for GitHub

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

Jump to bottom

Update WorkflowRun.Get Doc String #1346

Merged
merged 3 commits into from
Jan 14, 2024
Merged

Update WorkflowRun.Get Doc String #1346

merged 3 commits into from
Jan 14, 2024

Conversation

bplunkett-stripe
Copy link
Contributor

@bplunkett-stripe bplunkett-stripe commented Jan 12, 2024 •
edited
Loading

Doc string improvement - 😓

What was changed

Describe behavior of WorkflowRun.Get in the doc string.

Why?

This is a contract some folks might rely on, so it's good to encode it via a comment.

Checklist

  1. Closes
    No formal tickets

  2. How was this tested:

Comment

  1. Any docs updates needed?

@bplunkett-stripe
Update internal_workflow_client.go
4aa4de2 
Doc string improvement
@bplunkett-stripe bplunkett-stripe requested a review from a team as a code owner January 12, 2024 23:49
@CLAassistant
Copy link

CLAassistant commented Jan 12, 2024 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

mfateev
mfateev reviewed Jan 13, 2024
View reviewed changes
internal/internal_workflow_client.go
// execution is a success, or return corresponding error. If valuePtr is
// nil, the valuePtr will be ignored and only the corresponding error
// of the workflow will be returned (nil on workflow execution success).
// This is a blocking API.
Copy link
Member

Choose a reason for hiding this comment

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

Please update the GetWithOptions doc as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, good call.

@mfateev
Copy link
Member

mfateev commented Jan 13, 2024

Please sign the CLA to land.

@mfateev mfateev merged commit fac7f7f into temporalio:master Jan 14, 2024
12 checks passed
@bplunkett-stripe bplunkett-stripe deleted the describe-behavior-on-nil-ptr branch January 16, 2024 00:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Quinn-With-Two-Ns Quinn-With-Two-Ns Quinn-With-Two-Ns approved these changes

@mfateev mfateev Awaiting requested review from mfateev

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bplunkett-stripe @CLAassistant @mfateev @Quinn-With-Two-Ns