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

Feature - Add failed task #1195

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

BatanGaming
Copy link

@BatanGaming BatanGaming commented Mar 11, 2023

  • Added fail state for ProgressTask
  • Added failed rendering for: ProgressBar, PercentageColumn, SpinnerColumn, DownloadedColumn

Related issue - #314

Usage example:

using Spectre.Console;

await AnsiConsole.Progress()
    .Columns(
        new TaskDescriptionColumn(),
        new ProgressBarColumn
        {
            FailedStyle = new Style(Color.Red),
        },
        new PercentageColumn
        {
            FailedStype = new Style(Color.Red),
        },
        new SpinnerColumn
        {
            CompletedText = "Completed", 
            FailedText = "Failed",
            FailedStyle = new Style(Color.Red),
            CompletedStyle = new Style(Color.Green),
        },
        new DownloadedColumn()
    )
    .AutoClear(false)
    .StartAsync(async context =>
    {
        var goodTask = Task.Run(async () =>
        {
            var task = context.AddTask("Good task");
            var x = 0;
            while (x != 100)
            {
                task.Increment(1);
                x++;
                await Task.Delay(50);
            }
        });

        var badTask = Task.Run(async () =>
        {
            var task = context.AddTask("Bad task");
            try
            {
                var x = 0;
                while (x != 100)
                {
                    task.Increment(1);
                    x++;
                    await Task.Delay(100);
                    if (x == 30)
                    {
                        throw new Exception();
                    }
                }
            }
            catch (Exception ex)
            {
                task.StopTaskWithFailure();
            }
        });
        await Task.WhenAll(goodTask, badTask);
    });

Screenshot 2023-03-11 at 10 57 33


Please upvote 👍 this pull request if you are interested in it.

@BatanGaming
Copy link
Author

@microsoft-github-policy-service agree

/// <summary>
/// Gets a value indicating whether the task execution failed.
/// </summary>
public bool IsFailed { get; private set; }
Copy link

Choose a reason for hiding this comment

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

I would rather see a public setter on this, the current setup with StopTaskWithFailure implies the task always stops as soon as it's failed which I don't think is a reasonable assumption to make.

Copy link
Author

Choose a reason for hiding this comment

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

I don't think so. You named it right Completed with errors in the comment below and I think it's not related to Failed status. I think what you need is methods like AddError, GetErrors so then you can get occurred errors.
I wouldn't mix Failed and Completed statuses in any way because from my point of view Failed task cannot continue working. If some error occurred and task can continue working then I wouldn't call it Failed.

Copy link

Choose a reason for hiding this comment

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

But that feels like adding business-logic into a visual component whose goal it is to represents a process. I just want to visualize "The process encountered an error" and whether that stopped the process, or the process is still going is not related to the visual "error state".

Copy link
Author

Choose a reason for hiding this comment

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

For me it's just unintuitive to see failed job still going.

@patriksvensson Any chance to review this feature and join the discussion?

Copy link

@redoz redoz Jul 26, 2023

Choose a reason for hiding this comment

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

Imagine a CI/CD pipeline, you might have configured it so that even if a test fails, it still publishes the build artifacts. So then the process is in an error state, while still continuing.

In my case the progress bar represents a deployment where a system is deploying 10 different resources, one might fail, but that doesn't stop the deployment from deploying the other nine. (You could argue this is odd behavior, but the behavior is outside of my control, I simply schedule the deployment and monitor its progress).

I would also argue it's more in line with the current design, there are no restrictions on changing the Value, MaxValue or IsIndeterminate properties, what benefit is gained from restricting the Failed/Error state?

Copy link
Author

Choose a reason for hiding this comment

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

Your general example really looks odd for me. But for your particular case, I have something similar actually.
Screenshot 2023-07-26 at 15 55 27
So What I did - I created multiple ProgressTask that show the state of each individual task. Since we don't have any option to indicate that error occurred, I made custom widget that shows text state.
After the whole process is over it displays all errors that happened during it.

Back to the topic, my main idea is that
Failed status != Something bad happened during the process but process still goes on.
I agree, it'd be good to have something that indicates that something happened but I think it should be different status, not Failed. For your specific case, I'd suggest to separate deployments into independent tasks.

Copy link

Choose a reason for hiding this comment

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

This is a UI widget why enforce this limitation? If you want to, you can set Failed = true, and then stop the task, and you have what you want, a failed task that is no longer running.
In my case, I'd like to just say Failed = true and let it run until the underlying task is actually completed indicating that while this is still in progress, it encountered an issue. What's so wrong with having the option to do that?

@@ -46,7 +49,16 @@ protected override IEnumerable<Segment> Render(RenderOptions options, int maxWid
}

var bar = !options.Unicode ? AsciiBar : UnicodeBar;
var style = isCompleted ? FinishedStyle : CompletedStyle;
var style = CompletedStyle;
if (isCompleted)
Copy link

Choose a reason for hiding this comment

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

I'd like to see support for "Completed with errors", something can have run to completion while still encountering errors.

@github-actions github-actions bot added the ⭐ top pull request Top pull request. label Apr 24, 2024
@github-actions github-actions bot removed the ⭐ top pull request Top pull request. label May 16, 2024
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.

None yet

3 participants