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

fix(components): try catch subscription callbacks #167

Merged
merged 4 commits into from
Dec 13, 2024
Merged

Conversation

domire8
Copy link
Member

@domire8 domire8 commented Nov 19, 2024

Description

This PR improves the robustness of the component interfaces by try catching subscription callbacks in all cases. Until now, we "only" try catched additional user callbacks.

One might argue that we should only catch CoreException and not all exceptions as the user might want to throw exceptions on purpose but I can not imagine a setup where one would desire to throw an exception in a subscription callback.

Also, we could put the component in error state by putting the raise_error there but that as well, I'm just not sure if I want to introduce it, since we'd have to do that consistently in all the add_input variants. I like the idea of subscription callbacks just failing with a throttled log instead of raising the error on the component.

Review guidelines

Estimated Time of Review: 5 minutes

Checklist before merging:

  • Confirm that the relevant changelog(s) are up-to-date in case of any user-facing changes

Copy link
Member

@bpapaspyros bpapaspyros left a comment

Choose a reason for hiding this comment

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

Good addition indeed, just a suggestion on how we could perhaps address this

bpapaspyros
bpapaspyros previously approved these changes Nov 25, 2024
bpapaspyros
bpapaspyros previously approved these changes Dec 4, 2024
Copy link
Member

@bpapaspyros bpapaspyros left a comment

Choose a reason for hiding this comment

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

Looks good! thanks for adding the tests, I missed them in the previous review

bpapaspyros
bpapaspyros previously approved these changes Dec 9, 2024
eeberhard
eeberhard previously approved these changes Dec 13, 2024
Copy link
Member

@eeberhard eeberhard left a comment

Choose a reason for hiding this comment

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

I agree this is a nice thing for now to prevent unhandled exceptions. We can think about additional user requirements from there, but a user could also raise_error manually from within their callback so I think it's already quite good!

@domire8 domire8 dismissed stale reviews from eeberhard and bpapaspyros via 63a62e5 December 13, 2024 09:13
@domire8 domire8 merged commit 935db82 into main Dec 13, 2024
4 checks passed
@domire8 domire8 deleted the feat/try-callback branch December 13, 2024 09:45
@github-actions github-actions bot locked and limited conversation to collaborators Dec 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants