-
Notifications
You must be signed in to change notification settings - Fork 0
Refactor console I/O abstractions for better separation of concerns #45
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
Conversation
Renamed interfaces and implementations: - IProgressNotifier → IConsoleWriter (clearer naming for text output) - ConsoleProgressNotifier → ConsoleWriter Created new progress display abstraction: - Added IProgressDisplay, IProgressContext, IProgressTask interfaces - Implemented SpectreProgressDisplay for rich progress bars - Decouples progress bar rendering from text output concerns Abstracted all console access through centralized classes: - Removed direct Console.WriteLine calls (moved to IConsoleWriter) - Replaced direct AnsiConsole.Progress() with IProgressDisplay - Updated RipOptions.DisplayHelp() to use IConsoleWriter parameter - Refactored EncoderService to inject IProgressDisplay - Refactored DiscRipper to inject IProgressDisplay - Updated MakeMkvOutputHandler to use IProgressTask and IConsoleWriter - Updated Program.cs final output to use IConsoleWriter Benefits: - Improved testability (all I/O is mockable) - Better separation of concerns (text vs progress display) - Clearer interface naming that communicates intent - Consistent abstraction pattern throughout codebase - Spectre.Console usage isolated to implementation classes All 112 tests passing. Closes #44
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request refactors console I/O abstractions to improve separation of concerns, testability, and code maintainability by removing direct console access from business logic.
Changes:
- Renamed
IProgressNotifiertoIConsoleWriterandConsoleProgressNotifiertoConsoleWriterfor clearer naming - Introduced new abstractions
IProgressDisplay,IProgressContext, andIProgressTaskfor progress bar operations - Implemented
SpectreProgressDisplayto encapsulate Spectre.Console-specific progress display logic - Updated all services and metadata providers to use the renamed
IConsoleWriterinterface - Removed all direct
Console.WriteLineandAnsiConsoleusage from business logic classes - Updated dependency injection configuration in
Program.csto register new abstractions - Modified
RipOptions.DisplayHelp()to acceptIConsoleWriterparameter for better testability - Updated all test mocks to use the renamed
IConsoleWriterinterface
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/RipSharp/Abstractions/IConsoleWriter.cs | Renamed interface from IProgressNotifier with added documentation |
| src/RipSharp/Abstractions/IProgressDisplay.cs | New interfaces for progress display abstraction |
| src/RipSharp/Utilities/ConsoleWriter.cs | Renamed class from ConsoleProgressNotifier with added documentation |
| src/RipSharp/Utilities/SpectreProgressDisplay.cs | New Spectre.Console implementation of progress display |
| src/RipSharp/Utilities/ConsoleUserPrompt.cs | Updated to use renamed IConsoleWriter |
| src/RipSharp/Services/EncoderService.cs | Refactored to use IProgressDisplay, removed direct Spectre.Console usage |
| src/RipSharp/Services/DiscRipper.cs | Refactored to use IProgressDisplay, removed direct Spectre.Console usage |
| src/RipSharp/Services/DiscScanner.cs | Updated to use renamed IConsoleWriter |
| src/RipSharp/MakeMkv/MakeMkvOutputHandler.cs | Updated to use IProgressTask and IConsoleWriter abstractions |
| src/RipSharp/MakeMkv/ScanOutputHandler.cs | Updated to use renamed IConsoleWriter |
| src/RipSharp/Metadata/MetadataService.cs | Updated to use renamed IConsoleWriter |
| src/RipSharp/Metadata/TmdbMetadataProvider.cs | Updated to use renamed IConsoleWriter |
| src/RipSharp/Metadata/OmdbMetadataProvider.cs | Updated to use renamed IConsoleWriter |
| src/RipSharp/Metadata/TvdbMetadataProvider.cs | Updated to use renamed IConsoleWriter |
| src/RipSharp/Core/RipOptions.cs | Updated DisplayHelp to accept IConsoleWriter parameter, updated project name references |
| src/RipSharp/Core/Program.cs | Updated DI configuration and final output to use abstractions |
| src/RipSharp.Tests/Metadata/TmdbMetadataProviderTests.cs | Updated test mocks to use IConsoleWriter |
| src/RipSharp.Tests/Metadata/OmdbMetadataProviderTests.cs | Updated test mocks to use IConsoleWriter |
| src/RipSharp.Tests/Metadata/MetadataServiceTests.cs | Updated test mocks to use IConsoleWriter |
Comments suppressed due to low confidence (1)
src/RipSharp/Services/DiscRipper.cs:206
- Incorrect indentation: Lines 151-206 have one extra level of indentation. These lines should be aligned with line 149 (the previous line in the lambda body) since they're all part of the same code block within the lambda passed to ExecuteAsync. The extra indentation appears to be left over from the previous code structure.
var pollTask = Task.Run(async () =>
{
double lastSizeLocal = 0;
string? currentMkv = null;
while (!ripDone)
{
try
{
// Identify the mkv file being written for this title: the first new mkv not in existingFiles
if (currentMkv == null)
{
currentMkv = Directory
.EnumerateFiles(options.Temp!, "*.mkv")
.FirstOrDefault(f => !existingFiles.Contains(f));
}
if (currentMkv != null && expectedBytes > 0)
{
var size = new FileInfo(currentMkv).Length;
lastSizeLocal = Math.Max(lastSizeLocal, size);
task.Value = (long)Math.Min(expectedBytes, lastSizeLocal);
}
}
catch { }
await Task.Delay(1000);
}
});
var rawLogPath = Path.Combine(options.Temp!, $"makemkv_title_{titleId:D2}.log");
var handler = new MakeMkvOutputHandler(expectedBytes, idx, totalTitles, task, progressLogPath, rawLogPath, _notifier);
var exit = await _makeMkv.RipTitleAsync(options.Disc, titleId, options.Temp!,
onOutput: handler.HandleLine,
onError: errLine =>
{
if (!(errLine.StartsWith("PRGV:") || errLine.StartsWith("PRGC:")))
{
_notifier.Error(errLine);
}
handler.HandleLine(errLine);
});
ripDone = true;
try { await pollTask; } catch { }
if (exit != 0)
{
task.Description = $"[{ConsoleColors.Error}]Failed: Title {titleId}[/]";
task.StopTask();
_notifier.Error($"Failed to rip title {titleId}");
return;
}
if (handler.LastBytesProcessed < maxValue)
{
task.Value = maxValue;
}
task.StopTask();
});
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Overview
Refactored console I/O abstractions to improve code quality, testability, and separation of concerns by removing all direct console access.
Changes
Renamed Interfaces & Classes
IProgressNotifier→IConsoleWriter(better describes text output purpose)ConsoleProgressNotifier→ConsoleWriterNew Abstractions
IProgressDisplay: Interface for progress bar operationsIProgressContext: Interface for managing multiple progress tasksIProgressTask: Interface for individual progress task updatesSpectreProgressDisplay: Spectre.Console implementation of IProgressDisplayConsole Abstraction
Console.WriteLinecallsAnsiConsoleusage from business logicRipOptions.DisplayHelp()to acceptIConsoleWriterparameterEncoderServiceto injectIProgressDisplayDiscRipperto injectIProgressDisplayMakeMkvOutputHandlerto useIProgressTaskandIConsoleWriterProgram.csfinal output to useIConsoleWriterBenefits
✅ Improved testability - all I/O is mockable
✅ Better separation of concerns - text output vs progress display
✅ Clearer interface naming that communicates intent
✅ Consistent abstraction pattern throughout codebase
✅ Spectre.Console usage isolated to implementation classes only
Testing
Closes #44