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

Refactor common context handling pattern #9087

Merged
merged 10 commits into from
Feb 19, 2024

Conversation

radeusgd
Copy link
Member

Important Notes

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed, the GUI was tested when built using ./run ide build.

@radeusgd radeusgd added the CI: No changelog needed Do not require a changelog entry for this PR. label Feb 16, 2024
@radeusgd radeusgd self-assigned this Feb 16, 2024
Copy link
Member

@jdunkerley jdunkerley 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 but also looks like it is called virtually every single time with panic=False so feels like that should be the default.

@radeusgd
Copy link
Member Author

Looks good but also looks like it is called virtually every single time with panic=False so feels like that should be the default.

Until we implement something like #5430, it is very easy to 'lose' track of a dataflow error if the result of the operation is not threaded-through further. This is especially likely for side-effecting operations - which Output operations often can be.

Thus, as explained in the doc comment

         A dataflow error may be lost if the result of the action is not used,
        so raising a `Panic` is safer. However, when we know that the result is
        not discarded, the dataflow error is preferred.

I thought that it is better to deliberately keep the panic as a default. Then, switching to dataflow error mode has to be done fully consciously, and I assume the user of the function will only do so if they know it is safe.

If the dataflow error mode were the default, someone may use this function in a place where the result is discarded, causing very confusing behaviour (when the Output context is disabled, nothing would happen but there would be no error either).

@radeusgd radeusgd added the CI: Ready to merge This PR is eligible for automatic merge label Feb 19, 2024
Copy link
Contributor

@hubertp hubertp left a comment

Choose a reason for hiding this comment

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

Approving engine changes

@mergify mergify bot merged commit cf71a05 into develop Feb 19, 2024
24 of 25 checks passed
@mergify mergify bot deleted the wip/radeusgd/9080-refactor-context-when_enabled branch February 19, 2024 11:59
@enso-bot
Copy link

enso-bot bot commented Feb 20, 2024

Radosław Waśko reports a new STANDUP for yesterday (2024-02-19):

Progress: Fixing some unexpectedly failing tests, got the PR merged. Final touches for #9021. It should be finished by 2024-02-19.

Next Day: Next day I will be working on the #9021 task. Ensure tests pass for S3 refactor. Look into next tasks, probably #9022

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: No changelog needed Do not require a changelog entry for this PR. CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor: improve and use Context.if_enabled for the common Forbidden_Operation check
4 participants