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: Change the behavior of notify parameter in context to default to null by default #449

Conversation

TheoD02
Copy link
Contributor

@TheoD02 TheoD02 commented May 3, 2024

Description

The changes allow the notify property in the context to be set to null and this is the new default value.

Previously, the notify property could only be set to true or false. Now, it can be set to null, true, or false.

Here's what each value means:

  • false: Notifications are disabled globally.
  • null: Notifications are disabled for internal Castor run, with calls, but notify() calls triggered by the user will still show notifications.
  • true: All notifications will be shown.

This change provides more flexibility for controlling when notifications are shown.

Changes

#[AsTask(description: 'Sends a notification')]
function send_notify(): void
{
    // The two tasks below will don't send a notification
    with(
        callback: function () {
            run(['sleep', '0.1']); // Will not send a notification
            notify('Hello world! (not send)'); // Will not send a notification

            run(['sleep', '0.1'], notify: true); // Will send a notification
        },
        context: context()->withNotify(false)
    );

    // This will send a notification
    with(
        callback: function () {
            run(['sleep', '0.1']); // Will send a notification
            notify('Hello world! (send with true)'); // Will send a notification

            run(['sleep', '0.1'], notify: false); // Will not send a notification for this specific run
        },
        context: context()->withNotify(true)
    );

    // This will send a notification partially
    with(
        callback: function () {
            run(['sleep', '0.1']); // Will not send a notification
            notify('Hello world! (send with null)'); // Will send a notification

            run(['sleep', '0.1'], notify: true); // Will send a notification for this specific run
        },
        context: context()->withNotify(null)
    );

    // This will send a notification
    with(
        callback: function () {
            run(['sleep', '0.1'], notify: true); // Will send a notification
            notify('Hello world! (send with null)'); // Will send a notification
        },
        context: context()->withNotify(null)
    );
}

Resolve #444

CHANGELOG.md Outdated Show resolved Hide resolved
doc/going-further/helpers/notify.md Outdated Show resolved Hide resolved
doc/going-further/helpers/notify.md Outdated Show resolved Hide resolved
examples/notify.php Outdated Show resolved Hide resolved
examples/notify.php Outdated Show resolved Hide resolved
examples/notify.php Outdated Show resolved Hide resolved
src/Helper/Notifier.php Outdated Show resolved Hide resolved
@TheoD02 TheoD02 force-pushed the fix/change_behavior_of_notify_context_parameter branch from bfa1f2e to c883238 Compare May 3, 2024 14:02
@TheoD02
Copy link
Contributor Author

TheoD02 commented May 3, 2024

Feedbacks fixed 😎

Copy link
Member

@lyrixx lyrixx left a comment

Choose a reason for hiding this comment

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

👍🏼 Thanks

Copy link
Member

@pyrech pyrech left a comment

Choose a reason for hiding this comment

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

Just a small typo but LGTM

examples/notify.php Outdated Show resolved Hide resolved
@TheoD02 TheoD02 force-pushed the fix/change_behavior_of_notify_context_parameter branch from c883238 to 3b09fbb Compare May 3, 2024 14:22
Copy link
Member

@pyrech pyrech left a comment

Choose a reason for hiding this comment

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

Thanks

@pyrech pyrech merged commit 197a545 into jolicode:main May 3, 2024
9 checks passed
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.

notify context is not super clear about expected behavior
3 participants