Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

docs: added notes toApplicationError andFailureError exception docstrings.#718

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
GSmithApps wants to merge13 commits intomain
base:main
Choose a base branch
Loading
fromdocs-added-notes-to-exception-docstrings

Conversation

@GSmithApps
Copy link
Contributor

What was changed

Added notes to exception docstrings.

Why?

Increase clarity and dicsoverability.

@CLAassistant
Copy link

CLAassistant commentedJan 2, 2025
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 ourContributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let usrecheck it.

@GSmithAppsGSmithAppsforce-pushed thedocs-added-notes-to-exception-docstrings branch from2c6c45b to41d13a7CompareJanuary 2, 2025 22:52
Copy link
Member

@cretzcretz left a comment

Choose a reason for hiding this comment

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

@dandavison - any thoughts/suggestions on API docs for explaining exceptions vs README or docs.temporal.io?

Comment on lines 50 to 51
Any exception that does not extend this exception
is considered a Workflow Task Failure. These types of failures will cause the Workflow Task to be retried.
Copy link
Member

@cretzcretzJan 6, 2025
edited
Loading

Choose a reason for hiding this comment

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

It's actually more complicated than this since it is now configurable. We dedicate a section of our README to this athttps://github.com/temporalio/sdk-python?tab=readme-ov-file#exceptions. Ideally we don't have to duplicate so much in here and there considering they may not say the same things, as is the case already here where it is not explained that this is customizable.

@dandavison
Copy link
Contributor

I like the direction of this PR, but would like to work on the wording a bit before merging.

@dandavison - any thoughts/suggestions on API docs for explaining exceptions vs README or docs.temporal.io?

IMO the information added in this PR is basic and important enough that it should be in the code (API docs) as this PR does. Ideally it should also be accurately duplicated in the docs site. I think the fact that this is customizable should definitely be noted, but should not distract us from communicating the basic Temporal exception philosophy that failing the workflow execution requires special exceptions to be raised and that, by default, unhandled exceptions fail the task.

@GSmithApps
Copy link
ContributorAuthor

GSmithApps commentedJan 6, 2025
edited
Loading

This is awesome - thank you guys so much for looking at this.

Dan, I committed your suggestions 👍

One other thing I think we should include is a note about manually failing an Activity. For example, if a developer/user wants to fail an Activity Task Execution (or I guess the execution as a whole), shouldn't they use an Application Error? In our notes here, we only mention things about failing Workflow Executions.

Lastly, I think we'll have similar changes in the other SDKs, for example Go 👍

GSmithAppsand others added3 commitsJanuary 6, 2025 15:27
Co-authored-by: Dan Davison <dan.davison@temporal.io>
Co-authored-by: Dan Davison <dan.davison@temporal.io>
@GSmithApps
Copy link
ContributorAuthor

Please forgive me if I'm missing something obvious, but do either of you know why the pipeline is failing? I'm pretty sure I'm making the linter happy, and this is failing after 13 minutes and 10,000 lines of logs. Also, I tried rerunning it, and I'm getting the same error.

@dandavison
Copy link
Contributor

do either of you know why the pipeline is failing?

This isn't your fault! The test suite has some flakes which we need to address. Unfortunately for now we just have to re-run until they pass. (Not sure yet what happened with the features tests)

GSmithApps reacted with rocket emoji

@GSmithApps
Copy link
ContributorAuthor

Hi@dandavison , thanks for the note on the pipeline, and thanks for the notes in general!

I've tried five times, and I can't seem to get this last piece to pass. Maybe we can try tomorrow or something -- I'm not sure if you have any tricks up your sleeve.

In any case, I'm happy with this however it turns out, so please feel free to make any modifications or merge however you please. I'll stay out of your hair and let you and the experts take it from here if that works 👍🏼 Certainly please do reach out though if there's any remaining legwork -- I'd be happy to take care of it.

@GSmithAppsGSmithApps marked this pull request as ready for reviewJanuary 7, 2025 21:50
@GSmithAppsGSmithApps requested a review froma team as acode ownerJanuary 7, 2025 21:50
GSmithAppsand others added4 commitsFebruary 15, 2025 08:52
Co-authored-by: Dan Davison <dan.davison@temporal.io>
Co-authored-by: Dan Davison <dan.davison@temporal.io>
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@dandavisondandavisondandavison left review comments

@cretzcretzcretz left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@GSmithApps@CLAassistant@dandavison@cretz

[8]ページ先頭

©2009-2025 Movatter.jp