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.

Sign up for GitHub

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
Open

Feature - Add failed task #1195

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
a8d8b42
Add 'Failed' state to the ProgressTask
BatanGaming Mar 11, 2023
6d1b3de
Add failed rendering for ProgressBar
BatanGaming Mar 11, 2023
c0a7489
Add failed rendering for PercentageColumn
BatanGaming Mar 11, 2023
b88b3fe
Add failed rendering for SpinnerColumn
BatanGaming Mar 11, 2023
e724537
Add failed rendering for DownloadedColumn
BatanGaming Mar 11, 2023
c5eeeb9
Add missing XML documentation
BatanGaming Mar 11, 2023
120dc70
Set _maxWidth to null as in CompletedText
BatanGaming Mar 11, 2023
506c0a3
Fix style field name
BatanGaming Jul 5, 2023
b3dba12
Merge branch 'main' into feature/failed-task
BatanGaming Jul 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions src/Spectre.Console/Live/Progress/Columns/DownloadedColumn.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,16 @@ public sealed class DownloadedColumn : ProgressColumn
public override IRenderable Render(RenderOptions options, ProgressTask task, TimeSpan deltaTime)
{
var total = new FileSize(task.MaxValue);
var downloaded = new FileSize(task.Value, total.Unit);

if (task.IsFailed)
{
return new Markup(string.Format(
"[red]{0}/{1}[/] [red]{2}[/]",
downloaded.Format(Culture),
total.Format(Culture),
total.Suffix));
}

if (task.IsFinished)
{
Expand All @@ -24,8 +34,6 @@ public override IRenderable Render(RenderOptions options, ProgressTask task, Tim
}
else
{
var downloaded = new FileSize(task.Value, total.Unit);

return new Markup(string.Format(
"{0}[grey]/[/]{1} [grey]{2}[/]",
downloaded.Format(Culture),
Expand Down
74 changes: 44 additions & 30 deletions src/Spectre.Console/Live/Progress/Columns/PercentageColumn.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,31 +1,45 @@
namespace Spectre.Console;

/// <summary>
/// A column showing task progress in percentage.
/// </summary>
public sealed class PercentageColumn : ProgressColumn
{
/// <summary>
/// Gets or sets the style for a non-complete task.
/// </summary>
public Style Style { get; set; } = Style.Plain;

/// <summary>
/// Gets or sets the style for a completed task.
/// </summary>
public Style CompletedStyle { get; set; } = Color.Green;

/// <inheritdoc/>
public override IRenderable Render(RenderOptions options, ProgressTask task, TimeSpan deltaTime)
{
var percentage = (int)task.Percentage;
var style = percentage == 100 ? CompletedStyle : Style ?? Style.Plain;
return new Text($"{percentage}%", style).RightJustified();
}

/// <inheritdoc/>
public override int? GetColumnWidth(RenderOptions options)
{
return 4;
}
namespace Spectre.Console;

/// <summary>
/// A column showing task progress in percentage.
/// </summary>
public sealed class PercentageColumn : ProgressColumn
{
/// <summary>
/// Gets or sets the style for a non-complete task.
/// </summary>
public Style Style { get; set; } = Style.Plain;

/// <summary>
/// Gets or sets the style for a completed task.
/// </summary>
public Style CompletedStyle { get; set; } = Color.Green;

/// <summary>
/// Gets or sets style for a failed task.
/// </summary>
public Style FailedStyle { get; set; } = new Style(foreground: Color.Red);

/// <inheritdoc/>
public override IRenderable Render(RenderOptions options, ProgressTask task, TimeSpan deltaTime)
{
var percentage = (int)task.Percentage;
var style = Style;
if (task.IsFailed)
{
style = FailedStyle;
}
else if (percentage == 100)
{
style = CompletedStyle;
}

return new Text($"{percentage}%", style).RightJustified();
}

/// <inheritdoc/>
public override int? GetColumnWidth(RenderOptions options)
{
return 4;
}
}
7 changes: 7 additions & 0 deletions src/Spectre.Console/Live/Progress/Columns/ProgressBarColumn.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,11 @@ public sealed class ProgressBarColumn : ProgressColumn
/// </summary>
public Style IndeterminateStyle { get; set; } = ProgressBar.DefaultPulseStyle;

/// <summary>
/// Gets or sets the style of a failed progress bar.
/// </summary>
public Style FailedStyle { get; set; } = new Style(foreground: Color.Red);

/// <inheritdoc/>
public override IRenderable Render(RenderOptions options, ProgressTask task, TimeSpan deltaTime)
{
Expand All @@ -43,6 +48,8 @@ public override IRenderable Render(RenderOptions options, ProgressTask task, Tim
RemainingStyle = RemainingStyle,
IndeterminateStyle = IndeterminateStyle,
IsIndeterminate = task.IsIndeterminate,
FailedStyle = FailedStyle,
IsFailed = task.IsFailed,
};
}
}
25 changes: 25 additions & 0 deletions src/Spectre.Console/Live/Progress/Columns/SpinnerColumn.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ public sealed class SpinnerColumn : ProgressColumn
private int? _maxWidth;
private string? _completed;
private string? _pending;
private string? _failedText;

/// <inheritdoc/>
protected internal override bool NoWrap => true;
Expand Down Expand Up @@ -47,6 +48,20 @@ public string? CompletedText
}
}

/// <summary>
/// Gets or sets the text that should be shown instead
/// of the spinner once a task was marked as failed.
/// </summary>
public string? FailedText
{
get => _failedText;
set
{
_failedText = value;
_maxWidth = null;
}
}

/// <summary>
/// Gets or sets the text that should be shown instead
/// of the spinner before a task begins.
Expand Down Expand Up @@ -76,6 +91,11 @@ public string? PendingText
/// </summary>
public Style? Style { get; set; } = Color.Yellow;

/// <summary>
/// Gets or sets the failed style.
/// </summary>
public Style? FailedStyle { get; set; }

/// <summary>
/// Initializes a new instance of the <see cref="SpinnerColumn"/> class.
/// </summary>
Expand Down Expand Up @@ -105,6 +125,11 @@ public override IRenderable Render(RenderOptions options, ProgressTask task, Tim
return new Markup(PendingText ?? " ", PendingStyle ?? Style.Plain);
}

if (task.IsFailed)
{
return new Markup(FailedText ?? " ", FailedStyle ?? Style.Plain);
}

if (task.IsFinished)
{
return new Markup(CompletedText ?? " ", CompletedStyle ?? Style.Plain);
Expand Down
21 changes: 21 additions & 0 deletions src/Spectre.Console/Live/Progress/ProgressTask.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,11 @@ public double Value
/// </summary>
public bool IsIndeterminate { get; set; }

/// <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 •
edited
Loading

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?


/// <summary>
/// Initializes a new instance of the <see cref="ProgressTask"/> class.
/// </summary>
Expand Down Expand Up @@ -152,6 +157,22 @@ public void StopTask()
}
}

/// <summary>
/// Stops and marks the task as failed.
/// </summary>
public void StopTaskWithFailure()
{
lock (_lock)
{
var now = DateTime.Now;
StartTime ??= now;

StopTime = now;

IsFailed = true;
}
}

/// <summary>
/// Increments the task's value.
/// </summary>
Expand Down
16 changes: 14 additions & 2 deletions src/Spectre.Console/Widgets/ProgressBar.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ internal sealed class ProgressBar : Renderable, IHasCulture
private const int PULSESIZE = 20;
private const int PULSESPEED = 15;

public bool IsFailed { get; set; }

public double Value { get; set; }
public double MaxValue { get; set; } = 100;

Expand All @@ -20,6 +22,7 @@ internal sealed class ProgressBar : Renderable, IHasCulture
public Style FinishedStyle { get; set; } = Color.Green;
public Style RemainingStyle { get; set; } = Color.Grey;
public Style IndeterminateStyle { get; set; } = DefaultPulseStyle;
public Style FailedStyle { get; set; } = new Style(foreground: Color.Red);

internal static Style DefaultPulseStyle { get; } = new Style(foreground: Color.DodgerBlue1, background: Color.Grey23);

Expand All @@ -35,7 +38,7 @@ protected override IEnumerable<Segment> Render(RenderOptions options, int maxWid
var completedBarCount = Math.Min(MaxValue, Math.Max(0, Value));
var isCompleted = completedBarCount >= MaxValue;

if (IsIndeterminate && !isCompleted)
if (IsIndeterminate && !isCompleted && !IsFailed)
{
foreach (var segment in RenderIndeterminate(options, width))
{
Expand All @@ -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.

{
style = FinishedStyle;
}
else if (IsFailed)
{
style = FailedStyle;
}

var barCount = Math.Max(0, (int)(width * (completedBarCount / MaxValue)));

// Show value?
Expand Down