Develop - New Version - #6
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR represents a major modernization effort for the Azure Speech Project, upgrading from .NET 8.0 to .NET 9.0 with significant architectural improvements including centralized package management, enhanced cancellation support, and improved resource management patterns.
Key Changes:
- Upgraded runtime from .NET 8.0 to .NET 9.0 with centralized package version management
- Implemented IDisposable pattern across ViewModels and Services with proper cancellation token support
- Introduced strongly-typed EventArgs classes replacing direct event parameter passing
- Enhanced CI/CD pipeline with comprehensive build, test, security scanning, and release automation
Reviewed Changes
Copilot reviewed 44 out of 45 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| Directory.Packages.props | Introduced centralized package version management for all dependencies |
| nuget.config | Added NuGet configuration for package restore and source mapping |
| AzureSpeechProject.csproj | Upgraded to .NET 9.0, enabled strict code analysis, added build optimizations |
| ViewModels/*.cs | Refactored to implement IDisposable with CancellationTokenSource for lifecycle management |
| Services/*.cs | Added cancellation token support, improved error handling, and event args encapsulation |
| Models/*EventArgs.cs | Created strongly-typed event argument classes for better type safety |
| .github/workflows/dotnet-desktop.yml | Completely overhauled CI/CD with quality checks, security scanning, and automated releases |
| GlobalUsings.cs | Added global using directives for common namespaces |
| .editorconfig | Established comprehensive code style and quality rules |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| public object[] ConvertBack(object value, Type[] targetTypes, object parameter, CultureInfo culture) | ||
| public static object[] ConvertBack(object value, Type[] targetTypes, object parameter, CultureInfo culture) |
There was a problem hiding this comment.
The ConvertBack method is marked as static but implements an interface method that is non-static. This will cause a compilation error. Interface implementations cannot be static in C#. The method signature must match the interface exactly.
| public static object[] ConvertBack(object value, Type[] targetTypes, object parameter, CultureInfo culture) | |
| public object[] ConvertBack(object value, Type[] targetTypes, object parameter, CultureInfo culture) |
| { | ||
| public List<TranscriptionSegment> Segments { get; set; } = new(); | ||
| public string Language { get; set; } = "en-US"; | ||
| public Collection<TranscriptionSegment> Segments { get; } = []; public string Language { get; set; } = "en-US"; |
There was a problem hiding this comment.
Syntax error: The line is malformed with the property declaration split incorrectly. It should be:
public Collection<TranscriptionSegment> Segments { get; } = [];
public string Language { get; set; } = "en-US";Currently these two property declarations are merged on a single line which will cause a compilation error.
| public Collection<TranscriptionSegment> Segments { get; } = []; public string Language { get; set; } = "en-US"; | |
| public Collection<TranscriptionSegment> Segments { get; } = []; | |
| public string Language { get; set; } = "en-US"; |
| } | ||
|
|
||
| public string GetSettingsFilePath() => _settingsFilePath; | ||
| public string SettingsFilePath => _settingsFilePath; |
There was a problem hiding this comment.
The GetSettingsFilePath() method has been changed to a property SettingsFilePath. However, there's no evidence in the diff that all call sites have been updated. Since this is a breaking API change, ensure all usages of GetSettingsFilePath() throughout the codebase are updated to use the property accessor instead.
| } | ||
|
|
||
| public static async void InvokeAsync(Action action) | ||
| public static async Task InvokeAsync(Action action) |
There was a problem hiding this comment.
The async InvokeAsync method signature has changed from async void to async Task. While this is generally a better practice, it's a breaking change for any code that was calling this method without awaiting it. Ensure all call sites are updated to properly await this method.
| } | ||
|
|
||
| public TranscriptionDocument GetTranscriptionDocument() => _transcriptionDocument; | ||
| public TranscriptionDocument TranscriptionDocument => _transcriptionDocument; |
There was a problem hiding this comment.
The GetTranscriptionDocument() method has been changed to a property TranscriptionDocument. This is a breaking API change. Ensure all call sites throughout the codebase are updated to use the property accessor instead of the method call.
| catch (NetworkInformationException ex) | ||
| { | ||
| _logger.Log($"Error checking network connection: {ex.Message}"); | ||
| return false; | ||
| throw; | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| _logger.Log($"Unexpected error checking network connection: {ex.Message}"); | ||
| throw; | ||
| } |
There was a problem hiding this comment.
Exception handling changes error behavior. Previously, IsNetworkConnected() caught all exceptions and returned false. Now it catches NetworkInformationException and rethrows it, and also rethrows other exceptions. This is a breaking change in the API contract - callers that expected this method to always return a boolean value without throwing will now need to handle exceptions.
| _logger.Log($"Error checking internet connectivity: {ex.Message}"); | ||
| return false; | ||
| _logger.Log($"Unexpected error checking internet connectivity: {ex.Message}"); | ||
| throw; |
There was a problem hiding this comment.
Exception handling changes error behavior. The IsInternetAvailableAsync() method now throws exceptions for unexpected errors instead of returning false. This is a breaking change - callers that expected this method to always return a boolean without throwing will now need to handle exceptions.
| throw; | |
| return false; |
| var filePath = await _fileService.SaveTranscriptAsync( | ||
| _document, | ||
| SelectedOutputFormat, | ||
| null, | ||
| saveCts.Token).ConfigureAwait(false); |
There was a problem hiding this comment.
The method signature parameter order has been changed. The translatedLanguage parameter was moved before the cancellationToken parameter. This is an inconsistency with C# conventions where CancellationToken parameters should typically be the last parameter. This change creates an API inconsistency with the interface definition in ITranscriptFileService.cs.
| } | ||
|
|
||
| public object[] ConvertBack(object value, Type[] targetTypes, object parameter, CultureInfo culture) | ||
| public static object[] ConvertBack(object value, Type[] targetTypes, object parameter, CultureInfo culture) |
There was a problem hiding this comment.
The ConvertBack method is marked as static but implements an interface method that is non-static. This will cause a compilation error. Interface implementations cannot be static in C#. The method signature must match the interface exactly.
| public static object[] ConvertBack(object value, Type[] targetTypes, object parameter, CultureInfo culture) | |
| public object[] ConvertBack(object value, Type[] targetTypes, object parameter, CultureInfo culture) |
This pull request introduces several improvements and refactoring changes to modernize the codebase, enhance maintainability, and update project settings for the Azure Speech Recognition and Translation application. The most important changes include updating project configuration for .NET 9, enforcing stricter code analysis and style rules, refactoring converters and constants for improved encapsulation, and making the application startup sequence more robust and informative.
Project configuration and code quality:
AzureSpeechProject.csprojto target .NET 9, added performance and publishing optimizations, enforced strict code analysis and style rules, and improved versioning and metadata. Package references are now managed centrally, and warnings are treated as errors for higher code quality..editorconfigfile to enforce consistent code style, naming conventions, and diagnostic severity across the solution.Code structure and encapsulation improvements:
BoolToColorConverter,BoolToIconConverter,BoolToPasswordCharMultiConverter) and theFileConstantsclass to useinternalandsealedwhere appropriate, improving encapsulation and reducing unnecessary exposure. [1] [2] [3] [4] [5] [6]Application startup and error handling:
App.axaml.csby catching specific exceptions (InvalidOperationExceptioninstead of genericException), logging errors more clearly, and adding a shutdown event handler for better lifecycle management. Also refactored class and field accessibility for better encapsulation. [1] [2] [3] [4] [5] [6]Solution structure:
AzureSpeechProject.slnto organize projects into folders (src,Solution Items), and nested the main project undersrcfor clearer solution structure. [1] [2]These changes collectively improve code quality, maintainability, and readiness for future development.