Chore/extending filter#6
Merged
Merged
Conversation
- Removed deprecated test files, including `CliInputUtilitiesTests.cs` and `MenuHelperTests.cs`. - Updated tests to use C# 12 collection literals (`[...]`) for arrays and lists. - Renamed `FrameworkOptionsTests` to `OptionsTests` and `DivideByZeroExtensionsTests` to `MathExtensionsTests` to reflect class renaming. - Simplified object initialization with `new()` and improved null-checking. - Removed `version.json` and added new NuGet dependencies in `VisionaryCoder.Framework.Tests.csproj`. - Refactored `StorageServiceTests` and `QueryFilterExtensionsTests` for clarity and modern syntax. - General cleanup and alignment with modern C# practices.
Refactored filter operations by replacing `FilterOperator` with `FilterOperation` for unified handling and introduced specific enums for clarity. Enhanced collection filtering with support for `Any`, `All`, `HasElements`, and `IN` operations. Improved `FilterNode` extensibility and added XML documentation. Introduced `PocoFilterExecutionStrategy` and `EfFilterExecutionStrategy` for applying filters to in-memory collections and EF Core queries. Added `ServiceResultBase` for cleaner success/failure handling with functional-style methods. Integrated Azure Table and Queue Storage with `AzureTableStorageProvider`, `AzureQueueStorageProvider`, and their respective options and interfaces. Implemented CRUD, batch, and messaging operations. Enhanced dependency injection with named registrations for storage providers. Added `QueryFilter<T>` for reusable predicates and demonstrated usage with POCOs and EF Core. Modernized code with improved null handling, structured logging, and updated syntax. Updated tests for new filter operations and added `IN` operation tests. Removed legacy Azure storage code and unused files. Improved documentation with detailed XML comments.
Updated README.md to enhance clarity on repository structure, modularity, and future decomposition plans. Added examples for the new `IN` operator in the filtering subsystem. Introduced ADR-0005 to document the rationale and implementation details for the `IN` operator, including its translation for EF Core and POCO execution. Added new READMEs for the filtering subsystem and querying helpers, detailing their components, usage, and guidance for splitting them into standalone packages. These updates aim to improve developer onboarding, maintainability, and modular development practices.
There was a problem hiding this comment.
Pull Request Overview
This pull request performs extensive refactoring and modernization across the VisionaryCoder.Framework codebase. The changes focus on code modernization (C# 12+ features), test project configuration improvements, namespace reorganizations, and architectural improvements to filtering, configuration, and proxy components.
Key Changes:
- Modernization of array syntax from
new T[]to collection expressions[] - Test project enhancements with MSTest, FluentAssertions, and Moq
- Namespace reorganization for proxy interceptors and related components
- Addition of new configuration provider abstractions and implementations
- ServiceResult refactoring with base class extraction
Reviewed Changes
Copilot reviewed 293 out of 294 changed files in this pull request and generated 35 comments.
Show a summary per file
| File | Description |
|---|---|
| version.json | Deleted versioning configuration file |
| tests/.../VisionaryCoder.Framework.Tests.csproj | Enhanced test project with packages and analyzer settings |
| tests/.../Usings.cs | Added global usings for FluentAssertions and MSTest |
| tests/.../StorageServiceTests.cs | Modernized variable declarations (var → explicit types) |
| tests/.../*Tests.cs | Applied C# 12 collection expressions throughout test files |
| tests/.../AssemblyInfo.cs | Added parallel test execution configuration |
| src/.../VisionaryCoder.Framework.csproj | Restructured dependencies with labels and added new packages |
| src/.../Storage/*.cs | Refactored storage extensions and registration patterns |
| src/.../ServiceResult.cs | Consolidated ServiceResult types with base class |
| src/.../Proxy/ProxyExtensions.cs | CRITICAL SYNTAX ERROR - Malformed extension method |
| src/.../Proxy/Interceptors/*.cs | Added missing using directives throughout |
| src/.../Configuration/*.cs | Added new configuration provider abstraction layer |
Comments suppressed due to low confidence (10)
src/VisionaryCoder.Framework/Proxy/ProxyExtensions.cs:21
- Invalid C# syntax:
extension(IServiceCollection services)on line 14 is not valid. This appears to be a malformed attempt at an extension method. The method signature should bepublic static IServiceCollection AddProxyPipeline(this IServiceCollection services). The current code will not compile.
src/VisionaryCoder.Framework/Proxy/ProxyExtensions.cs:38 - Missing
this IServiceCollection servicesparameter. Extension methods require thethiskeyword on the first parameter. Should bepublic static IServiceCollection AddProxyTransport<TTransport>(this IServiceCollection services).
src/VisionaryCoder.Framework/Proxy/ProxyExtensions.cs:51 - Missing
this IServiceCollection servicesparameter. Extension methods require thethiskeyword on the first parameter. Should bepublic static IServiceCollection AddProxyInterceptor<TInterceptor>(this IServiceCollection services, ServiceLifetime lifetime = ServiceLifetime.Transient).
src/VisionaryCoder.Framework/Proxy/ProxyExtensions.cs:58 - Extra closing brace on line 58. The class already closes on line 57. This will cause a compilation error.
src/VisionaryCoder.Framework/Proxy/Interceptors/NullLoggingInterceptor.cs:6 - Duplicate copyright header (lines 1-2 and 5-6). Remove the duplicate copyright notice to maintain clean code.
tests/VisionaryCoder.Framework.Tests/Caching/Providers/NullCacheKeyProviderTests.cs:133 - Variable result2 is always null at this dereference.
tests/VisionaryCoder.Framework.Tests/Caching/Providers/NullCacheKeyProviderTests.cs:134 - Variable result3 is always null at this dereference.
src/VisionaryCoder.Framework/Proxy/Interceptors/Configuration/Local/LocalConfigurationProvider.cs:83 - Generic catch clause.
src/VisionaryCoder.Framework/Proxy/Interceptors/Configuration/Local/LocalConfigurationProvider.cs:117 - Generic catch clause.
src/VisionaryCoder.Framework/Proxy/Interceptors/Authentication/Providers/DefaultUserContextProvider.cs:310 - Both branches of this 'if' statement write to the same variable - consider using '?' to express intent better.
Comment on lines
+1
to
+7
| // QueryFilterDemo.cs | ||
|
|
||
| namespace VisionaryCoder.Framework.Querying.Sample; | ||
| // QueryFilter, QueryFilterExtensions | ||
|
|
||
| // POCO models | ||
| public class Order { public int Id { get; set; } public decimal Total { get; set; } } |
There was a problem hiding this comment.
Incorrect file header comment. Line 1 references 'QueryFilterDemo.cs' but this is 'Order.cs'. Also, lines 4 and 6 contain unclear comments. Consider removing or clarifying these comments.
| public void Validate() | ||
| { | ||
| ArgumentException.ThrowIfNullOrWhiteSpace(ContainerName, nameof(ContainerName)); | ||
| ArgumentException.ThrowIfNullOrWhiteSpace(ContainerName); |
There was a problem hiding this comment.
The ArgumentException.ThrowIfNullOrWhiteSpace method requires a paramName argument to provide clear error messages. Should be ArgumentException.ThrowIfNullOrWhiteSpace(ContainerName, nameof(ContainerName)) to match the pattern used in other validation methods.
| if (UseManagedIdentity) | ||
| { | ||
| ArgumentException.ThrowIfNullOrWhiteSpace(StorageAccountUri, nameof(StorageAccountUri)); | ||
| ArgumentException.ThrowIfNullOrWhiteSpace(StorageAccountUri); |
There was a problem hiding this comment.
Missing paramName argument for proper error messaging. Should be ArgumentException.ThrowIfNullOrWhiteSpace(StorageAccountUri, nameof(StorageAccountUri)).
| else | ||
| { | ||
| ArgumentException.ThrowIfNullOrWhiteSpace(ConnectionString, nameof(ConnectionString)); | ||
| ArgumentException.ThrowIfNullOrWhiteSpace(ConnectionString); |
There was a problem hiding this comment.
Missing paramName argument for proper error messaging. Should be ArgumentException.ThrowIfNullOrWhiteSpace(ConnectionString, nameof(ConnectionString)).
Comment on lines
+202
to
208
| foreach (TKey key in keys) | ||
| { | ||
| ArgumentNullException.ThrowIfNull(keys, nameof(keys)); | ||
| int count = 0; | ||
| foreach (TKey key in keys) | ||
| if (dictionary.Remove(key)) | ||
| { | ||
| if (dictionary.Remove(key)) | ||
| { | ||
| count++; | ||
| } | ||
| count++; | ||
| } | ||
| return count; | ||
| } |
There was a problem hiding this comment.
This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
Comment on lines
+92
to
+96
| catch (Exception ex) | ||
| { | ||
| Logger.LogError(ex, "Failed to get all configuration values"); | ||
| return new Dictionary<string, object?>(); | ||
| } |
There was a problem hiding this comment.
Generic catch clause.
Comment on lines
+128
to
+132
| catch (Exception ex) | ||
| { | ||
| Logger.LogError(ex, "Failed to refresh configuration"); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Generic catch clause.
Comment on lines
+56
to
+61
| catch (Exception ex) | ||
| { | ||
| logger.LogError(ex, "Failed to retrieve secret '{SecretName}' from Key Vault", name); | ||
| // Don't cache failures, but don't rethrow to allow fallback behavior | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Generic catch clause.
Comment on lines
+136
to
139
| catch (Exception ex) | ||
| { | ||
| onFailure(ErrorMessage ?? "Unknown error", Exception); | ||
| return ServiceResult<TNew>.Failure(ex); | ||
| } |
There was a problem hiding this comment.
Generic catch clause.
Comment on lines
+159
to
+162
| catch (Exception ex) | ||
| { | ||
| return ServiceResult<TNew>.Failure(ex); | ||
| } |
There was a problem hiding this comment.
Generic catch clause.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.