V9.0.0/dotnet rc2 #5
Conversation
…, include instance in converters) and cleanup of redundancies
…null values (consistency with other formatters)
WalkthroughThe changes in this pull request involve significant updates to the Codebelt.Extensions libraries, particularly version 9.0.0. Key modifications include the removal of .NET 6 support, updates to method signatures, and enhancements to YAML serialization options. New methods such as Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5 +/- ##
==========================================
+ Coverage 86.66% 89.52% +2.85%
==========================================
Files 15 17 +2
Lines 555 611 +56
Branches 47 57 +10
==========================================
+ Hits 481 547 +66
+ Misses 74 64 -10 ☔ View full report in Codecov by Sentry. |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Outside diff range and nitpick comments (27)
src/Codebelt.Extensions.AspNetCore.Text.Yaml/Bootstrapper.cs (2)
8-9: Consider consistent naming convention for private fields.The private fields are correctly declared for thread-safe initialization. For consistency, consider renaming
PadLockto_padLockto match the naming convention used for_initialized.- private static readonly object PadLock = new(); + private static readonly object _padLock = new();
1-29: Overall, excellent implementation with room for minor improvements.The
Bootstrapperclass is well-designed and effectively implements thread-safe initialization for YAML formatting options. The code is concise, focused, and follows good practices.To further enhance the code:
- Consider adding XML documentation to the class and method to improve maintainability and clarity for other developers.
- Implement the suggested minor improvements from previous comments (naming convention and method extraction).
Here's an example of how you could add XML documentation:
/// <summary> /// Provides thread-safe initialization for YAML formatting options. /// </summary> internal static class Bootstrapper { // ... existing fields ... /// <summary> /// Initializes the YAML formatting options by adding necessary converters. /// This method is thread-safe and ensures that initialization occurs only once. /// </summary> internal static void Initialize() { // ... existing implementation ... } // ... suggested private method ... }src/Codebelt.Extensions.YamlDotNet/Diagnostics/ExceptionDescriptorExtensions.cs (1)
24-25: Approve the formatter usage, but suggest adding a comment.The change to use
YamlFormatterinstead ofExceptionDescriptorConverteraligns with the PR objectives and provides a more flexible approach to YAML serialization.Consider adding a brief comment explaining the purpose of
ToEncodedString()for clarity. For example:// Convert the serialized YAML to a properly encoded string representation return formatter.Serialize(descriptor).ToEncodedString();.nuget/Codebelt.Extensions.AspNetCore.Mvc.Formatters.Text.Yaml/PackageReleaseNotes.txt (1)
8-11: Approve new features and suggest additional documentationThe new features and changes to existing methods enhance the package's capabilities, particularly in error handling and problem details support. These changes align well with the PR objectives.
Consider the following suggestions to improve the release notes and documentation:
- Provide brief examples or use cases for the new
AddProblemDetailsConvertermethod.- Explain the benefits of the new
InstanceandTraceIdparameters inAddHttpExceptionDescriptorConverter.- Clarify how the
ProblemDetailssupport inAddYamlExceptionResponseFormatterimproves error reporting.Adding this information will help users understand the value of these changes and how to implement them effectively.
test/Codebelt.Extensions.YamlDotNet.Tests/Formatters/YamlFormatterOptionsTest.cs (3)
62-62: LGTM. Consider adding a comment about the default converter.The assertion correctly checks for the new default behavior where
YamlFormatterOptionsincludes one converter by default. This change aligns with the updated implementation.Consider adding a comment explaining what this default converter is and why it's included by default. This would improve the test's readability and make it easier for other developers to understand the expected behavior.
67-67: LGTM. Consider adding comments about the added converters.The assertion correctly checks for the updated behavior where
RefreshWithConverterDependenciesadds two more converters, bringing the total to three. This change aligns with the updated implementation.Consider adding comments explaining what these three converters are (the default one and the two added by
RefreshWithConverterDependencies). This would improve the test's readability and make it easier for other developers to understand the expected behavior after the refresh operation.
62-67: Summary: Updated assertions reflect changes in YamlFormatterOptions behaviorThese changes to the unit tests indicate important updates in the
YamlFormatterOptionsclass:
- The default state now includes one converter instead of zero.
- After calling
RefreshWithConverterDependencies, there are now three converters instead of two.These modifications align with the PR objectives of enhancing YAML conversion capabilities. They suggest that the
YamlFormatterOptionsclass has been updated to provide more robust default functionality and additional converters when refreshed.As these changes might affect other parts of the codebase that interact with
YamlFormatterOptions, ensure that:
- The documentation for
YamlFormatterOptionsis updated to reflect these new defaults.- Any code that relies on the previous behavior (zero default converters, two after refresh) is reviewed and updated if necessary.
- Consider adding integration tests to verify that these changes work correctly in the context of the broader system.
src/Codebelt.Extensions.YamlDotNet/Converters/YamlConverter.cs (2)
24-27: Approve the property change with suggestions for documentation.The change from
YamlFormatterOptionstoFormatters.YamlFormatterappears to be a good refactoring that could improve encapsulation and modularity. However, as this is a breaking change, it's important to ensure that users of the library can easily migrate their code.Consider the following suggestions:
- Add a comment explaining the rationale behind this change.
- Update the class-level documentation to mention this significant change.
- Create a migration guide in the project's documentation to help users transition from
YamlFormatterOptionstoYamlFormatter.Example class-level documentation addition:
/// <summary> /// Converts an object to and from YAML (YAML ain't markup language). /// </summary> /// <remarks> /// Version 9.0.0 introduced a breaking change, replacing YamlFormatterOptions with YamlFormatter. /// See the migration guide for details on updating your code. /// </remarks> public abstract class YamlConverter : IYamlTypeConverter
Action Required: Incomplete Refactoring of
SetPropertyNameDetectedThe removal or modification of
SetPropertyNameinYamlConverter.cshas not been fully propagated throughout the codebase. Multiple files still referenceSetPropertyName, which may lead to runtime errors or unexpected behavior.Affected Files:
YamlFormatterOptionsExtensions.csExceptionDescriptorConverter.csExceptionConverter.csYamlConverterExtensions.csPlease address these lingering references to ensure consistency and prevent potential issues.
🔗 Analysis chain
Line range hint
1-93: Overall assessment: Approve with suggestions for documentation and testing.The changes to this file, particularly the introduction of
YamlFormatterand the removal ofSetPropertyName, appear to be part of a significant refactoring effort. While these changes likely improve the design and functionality of the YAML conversion process, they introduce breaking changes that require careful handling.To ensure a smooth transition for users of this library:
- Thoroughly document all breaking changes in the release notes and migration guide.
- Consider adding deprecation warnings in the previous version to help users prepare for these changes.
- Implement comprehensive unit tests to verify that all YAML conversion scenarios still work as expected with the new
YamlFormatter.- Consider providing extension methods or utility functions to ease the transition from the old API to the new one.
To ensure the changes haven't introduced any regressions, please run the following command to check for any failing tests related to YAML conversion:
This will help verify that the core functionality remains intact after these significant changes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Run tests related to YAML conversion dotnet test --filter "FullyQualifiedName~Yaml"Length of output: 6482
.nuget/Codebelt.Extensions.YamlDotNet/PackageReleaseNotes.txt (2)
13-14: Consider adding more details about the new featureThe addition of the YamlConverterExtensions class with the AddFailureConverter method is a valuable new feature. However, it would be beneficial to provide more information about the purpose and use case of this new extension method. This will help users understand when and how to utilize this new functionality effectively.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~14-~14: This verb does not appear to agree with the subject. Consider using a different form.
Context: ...ns.YamlDotNet.Converters namespace that consist of one extension method for the ICollec...(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
14-14: Fix grammatical issues in the release notesThere are two minor grammatical issues in the release notes:
- On line 14, "consist" should be "consists" to agree with the singular subject "class".
- On line 17, "omit" should be "omits" to agree with the singular subject "class".
Please update these lines to correct the verb agreement:
Also applies to: 17-17
🧰 Tools
🪛 LanguageTool
[uncategorized] ~14-~14: This verb does not appear to agree with the subject. Consider using a different form.
Context: ...ns.YamlDotNet.Converters namespace that consist of one extension method for the ICollec...(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
CHANGELOG.md (4)
14-17: LGTM! Consider adding documentation for new methods.The additions of
YamlConverterExtensionsclasses and their respective methods (AddFailureConverterandAddProblemDetailsConverter) are valuable enhancements to the library's YAML conversion capabilities.To improve developer experience, consider adding XML documentation comments for these new methods, explaining their purpose, parameters, and return values.
19-26: Approved changes. Consider highlighting breaking changes more prominently.The changes introduced in version 9.0.0 align well with modern coding practices and enhance the library's flexibility. The switch to camelCase naming convention and omitting null values by default are particularly noteworthy improvements.
To ensure users are well-informed about the breaking changes, consider adding a separate "Breaking Changes" section at the top of the changelog entry for version 9.0.0. This will make it easier for developers to quickly identify and address potential issues when upgrading.
Line range hint
30-37: LGTM! Consider adding usage examples for new features.The additions and changes in version 8.4.0, particularly the new
ExceptionDescriptorExtensionsclass and the enhancements to theYamlFormatterclass, are valuable improvements to the library's exception handling and YAML formatting capabilities.To help developers quickly understand and adopt these new features, consider adding brief usage examples or links to detailed documentation for:
- Converting an ExceptionDescriptor to YAML
- Excluding non-essential properties from serialization
- Using the new action delegate factory for YAML deserialization
🧰 Tools
🪛 LanguageTool
[uncategorized] ~12-~12: Use a comma before ‘so’ if it connects two independent clauses (unless they are closely connected and short).
Context: ... introduced with .NET 9 preview releases so the final release is production ready t...(COMMA_COMPOUND_SENTENCE_2)
Line range hint
1-37: Excellent changelog. Minor suggestions for improvement.The changelog effectively documents the changes in versions 9.0.0 and 8.4.0, following the Keep a Changelog format and Semantic Versioning. It's well-structured and informative, clearly distinguishing between different types of changes.
To further enhance the changelog:
- Consider adding a "Breaking Changes" section at the top of the 9.0.0 entry to highlight critical changes.
- Add brief usage examples or links to documentation for new features.
- Include a brief explanation of the rationale behind major changes, especially for breaking changes.
These additions would make the changelog even more valuable for users upgrading their dependencies.🧰 Tools
🪛 LanguageTool
[uncategorized] ~12-~12: Use a comma before ‘so’ if it connects two independent clauses (unless they are closely connected and short).
Context: ... introduced with .NET 9 preview releases so the final release is production ready t...(COMMA_COMPOUND_SENTENCE_2)
src/Codebelt.Extensions.YamlDotNet/Formatters/YamlFormatterOptions.cs (1)
31-34: LGTM! Consider documenting the behavior change.The addition of the static constructor and the
DefaultConvertersstatic property is a good improvement. It ensures that default converters are always initialized when the class is first accessed, providing a consistent starting point for all instances.However, consider the following points:
- Document this new behavior in the class documentation, as it might affect users who expect
DefaultConvertersto be null initially.- Consider making
DefaultConvertersa read-only property to prevent unintended modifications:This would eliminate the need for the static constructor while maintaining the desired behavior.public static Action<IList<YamlConverter>> DefaultConverters { get; } = list => list.AddFailureConverter();src/Codebelt.Extensions.YamlDotNet/Formatters/YamlFormatter.cs (2)
130-130: LGTM! Consider adding a comment for clarity.The change from
FormatterOptionstoFormatterlooks good. It ensures thatYamlConverterinstances have a direct reference to the currentYamlFormatter, which is likely part of a broader refactoring to improve the relationship between these classes.Consider adding a brief comment explaining why this assignment is necessary, e.g.:
// Provide the converter with a reference to this formatter for advanced scenarios if (converter is YamlConverter yamlConverter) { yamlConverter.Formatter = this; }
199-199: LGTM! Consider maintaining consistency with UseSerializerBuilder.The change is good and consistent with the modification in
UseSerializerBuilder. It ensures thatYamlConverterinstances have a reference to the currentYamlFormatterduring deserialization as well.For consistency, consider adding the same comment as suggested for the
UseSerializerBuildermethod:// Provide the converter with a reference to this formatter for advanced scenarios if (converter is YamlConverter yamlConverter) { yamlConverter.Formatter = this; }test/Codebelt.Extensions.YamlDotNet.Tests/Diagnostics/ExceptionDescriptorExtensionsTest.cs (2)
38-41: LGTM: Improved YAML serialization configuration.The changes to the
ToYamlmethod call enhance the explicitness of the YAML serialization options. SettingPascalCaseNamingConvention.Instanceas the naming convention is consistent with the test expectations and improves the readability of the serialized output.For consistency with other parts of the codebase, consider using an expression-bodied member for the lambda:
-var sut2 = sut1.ToYaml(o => -{ - o.SensitivityDetails = FaultSensitivityDetails.All; - o.Settings.NamingConvention = PascalCaseNamingConvention.Instance; -}); +var sut2 = sut1.ToYaml(o => +{ + o.SensitivityDetails = FaultSensitivityDetails.All; + o.Settings.NamingConvention = PascalCaseNamingConvention.Instance; +});
87-91: LGTM: Enhanced flexibility in YAML serialization configuration.The changes to the
ToYamlmethod call improve the flexibility of the YAML serialization options configuration. SettingPascalCaseNamingConvention.Instanceas the naming convention is consistent with the previous method and enhances the readability of the serialized output. The use of theoptionsparameter forSensitivityDetailsmaintains the parameterized nature of the test.For consistency with other parts of the codebase and to improve readability, consider using an expression-bodied member for the lambda:
-var sut2 = sut1.ToYaml(o => -{ - o.SensitivityDetails = options.SensitivityDetails; - o.Settings.NamingConvention = PascalCaseNamingConvention.Instance; -}); +var sut2 = sut1.ToYaml(o => +{ + o.SensitivityDetails = options.SensitivityDetails; + o.Settings.NamingConvention = PascalCaseNamingConvention.Instance; +});test/Codebelt.Extensions.YamlDotNet.Tests/YamlSerializerTest.cs (2)
39-43: LGTM: Serialization settings updated appropriatelyThe changes to the serialization settings are appropriate:
- Adding
PascalCaseNamingConvention.Instanceensures consistent naming in the serialized output.- The
YamlConverterFactory.Create<DateTime>lambda has been updated to match the new method signature.Consider renaming the unused parameter
_to a more descriptive name (e.g.,context) or use_as a prefix (e.g.,_context) to indicate that it's intentionally unused. This improves code readability and adheres to common C# conventions for unused parameters.
242-243: LGTM: Consistent serialization settings across test methodsThe changes to the serialization settings in this method are consistent with those made in other test methods:
- Adding
PascalCaseNamingConvention.Instanceensures consistent naming in the serialized output.- The
YamlConverterFactory.Create<DateTime>lambda has been updated to match the new method signature.As suggested earlier, consider renaming the unused parameter
_to a more descriptive name (e.g.,context) or use_as a prefix (e.g.,_context) to indicate that it's intentionally unused. This improves code readability and maintains consistency across all test methods.src/Codebelt.Extensions.YamlDotNet/YamlSerializerOptions.cs (1)
93-93: Summary: Significant changes to default serialization behaviorThe changes to
NamingConventionandValuesHandlingdefaults represent a shift towards aligning YAML serialization more closely with common JSON practices. While these changes may improve interoperability and produce more compact output, they are breaking changes that could impact existing integrations.Recommendations:
- Ensure these changes are prominently documented in release notes and migration guides.
- Consider adding configuration options to allow users to revert to the old behavior if needed for backwards compatibility.
- Update all relevant documentation, especially sections dealing with serialization defaults.
- Review and update any internal usage of this class to ensure it aligns with the new defaults.
- Consider adding unit tests to verify the new default behaviors and to catch any unintended side effects.
Given the breaking nature of these changes, it might be beneficial to introduce a new major version of the library. This would clearly signal to users that there are significant changes and allow for a smoother transition period where both old and new behaviors could potentially be supported.
Also applies to: 101-101
src/Codebelt.Extensions.YamlDotNet/YamlConverterFactory.cs (4)
Line range hint
14-20: Update XML documentation to include 'YamlFormatter' parameterThe method
Create<T>now includesYamlFormatterin thewriterandreaderdelegates. The XML documentation for thewriterandreaderparameters does not reflect this change. Please update the documentation to describe theYamlFormatterparameter in the delegates.
Line range hint
25-32: Update XML documentation forCreate<T>overloadSimilarly, the
Create<T>method with apredicateparameter now haswriterandreaderdelegates includingYamlFormatter. Please ensure the XML comments for these parameters are updated accordingly.
Line range hint
37-44: Update XML documentation for non-genericCreatemethodThe
Createmethod now includesYamlFormatterin thewriterandreaderdelegates. Update the XML documentation for thewriterandreaderparameters to reflect the new delegate signatures.
Line range hint
48-55: Update XML documentation forCreatemethod with predicateThe
Createmethod with apredicateparameter has updatedwriterandreaderdelegates includingYamlFormatter. Please update the XML documentation to include descriptions of theYamlFormatterparameter.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (28)
- .nuget/Codebelt.Extensions.AspNetCore.Mvc.Formatters.Text.Yaml/PackageReleaseNotes.txt (1 hunks)
- .nuget/Codebelt.Extensions.YamlDotNet/PackageReleaseNotes.txt (2 hunks)
- CHANGELOG.md (1 hunks)
- Directory.Build.props (1 hunks)
- src/Codebelt.Extensions.AspNetCore.Mvc.Formatters.Text.Yaml/Codebelt.Extensions.AspNetCore.Mvc.Formatters.Text.Yaml.csproj (1 hunks)
- src/Codebelt.Extensions.AspNetCore.Text.Yaml/Bootstrapper.cs (1 hunks)
- src/Codebelt.Extensions.AspNetCore.Text.Yaml/Codebelt.Extensions.AspNetCore.Text.Yaml.csproj (1 hunks)
- src/Codebelt.Extensions.AspNetCore.Text.Yaml/Converters/YamlConverterExtensions.cs (2 hunks)
- src/Codebelt.Extensions.AspNetCore.Text.Yaml/Formatters/ServiceCollectionExtensions.cs (3 hunks)
- src/Codebelt.Extensions.YamlDotNet/Codebelt.Extensions.YamlDotNet.csproj (1 hunks)
- src/Codebelt.Extensions.YamlDotNet/Converters/ExceptionConverter.cs (1 hunks)
- src/Codebelt.Extensions.YamlDotNet/Converters/ExceptionDescriptorConverter.cs (3 hunks)
- src/Codebelt.Extensions.YamlDotNet/Converters/YamlConverter.cs (1 hunks)
- src/Codebelt.Extensions.YamlDotNet/Converters/YamlConverterExtensions.cs (1 hunks)
- src/Codebelt.Extensions.YamlDotNet/Diagnostics/ExceptionDescriptorExtensions.cs (1 hunks)
- src/Codebelt.Extensions.YamlDotNet/Formatters/YamlFormatter.cs (2 hunks)
- src/Codebelt.Extensions.YamlDotNet/Formatters/YamlFormatterOptions.cs (1 hunks)
- src/Codebelt.Extensions.YamlDotNet/YamlConverterFactory.cs (8 hunks)
- src/Codebelt.Extensions.YamlDotNet/YamlSerializerOptions.cs (1 hunks)
- test/Codebelt.Extensions.AspNetCore.Text.Yaml.Tests/AuthorizationResponseHandlerTest.cs (2 hunks)
- test/Codebelt.Extensions.AspNetCore.Text.Yaml.Tests/Codebelt.Extensions.AspNetCore.Text.Yaml.Tests.csproj (1 hunks)
- test/Codebelt.Extensions.AspNetCore.Text.Yaml.Tests/ServiceCollectionExtensions.cs (1 hunks)
- test/Codebelt.Extensions.YamlDotNet.Tests/Codebelt.Extensions.YamlDotNet.Tests.csproj (1 hunks)
- test/Codebelt.Extensions.YamlDotNet.Tests/Converters/ExceptionDescriptorConverterTest.cs (2 hunks)
- test/Codebelt.Extensions.YamlDotNet.Tests/Diagnostics/ExceptionDescriptorExtensionsTest.cs (3 hunks)
- test/Codebelt.Extensions.YamlDotNet.Tests/Formatters/YamlFormatterOptionsTest.cs (1 hunks)
- test/Codebelt.Extensions.YamlDotNet.Tests/YamlSerializerTest.cs (4 hunks)
- testenvironments.json (1 hunks)
✅ Files skipped from review due to trivial changes (2)
- test/Codebelt.Extensions.YamlDotNet.Tests/Codebelt.Extensions.YamlDotNet.Tests.csproj
- testenvironments.json
🧰 Additional context used
🪛 LanguageTool
.nuget/Codebelt.Extensions.YamlDotNet/PackageReleaseNotes.txt
[grammar] ~8-~8: You’ve repeated a verb. Did you mean to only write one of them?
Context: ...ort for TFM .NET 6 (LTS)
Breaking Changes
- CHANGED ToYaml method on the ExceptionDescripto...
(REPEATED_VERBS)
[uncategorized] ~10-~10: The grammatical number of this noun doesn’t look right. Consider replacing it.
Context: ... YamlFormatter parameter in all factory methods
- CHANGED YamlConverter class in the ...
(AI_EN_LECTOR_REPLACEMENT_NOUN_NUMBER)
[uncategorized] ~14-~14: This verb does not appear to agree with the subject. Consider using a different form.
Context: ...ns.YamlDotNet.Converters namespace that consist of one extension method for the ICollec...(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
[uncategorized] ~17-~17: This verb does not appear to agree with the subject. Consider using a different form.
Context: ...ng convention instead of PascalCase and omit null values by default
Version 8.4....(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
🔇 Additional comments (49)
test/Codebelt.Extensions.AspNetCore.Text.Yaml.Tests/Codebelt.Extensions.AspNetCore.Text.Yaml.Tests.csproj (1)
9-11: LGTM! Package references updated consistently.The package reference updates look good and are consistent with the project's target frameworks. Here are some observations:
- Cuemon packages (AspNetCore.Authentication and AspNetCore) are updated to the same preview version (9.0.0-preview.11).
- Codebelt.Extensions.Xunit.Hosting.AspNetCore is updated to 9.0.0-preview.10, which might be intentional due to different release cycles.
To ensure compatibility, please verify that all tests pass with these updated packages. Run the following command in the project directory:
If any issues arise during the build process, they may indicate incompatibilities with the new package versions.
src/Codebelt.Extensions.YamlDotNet/Codebelt.Extensions.YamlDotNet.csproj (1)
13-14: LGTM: Package references updated.The package references for Cuemon.Extensions.Reflection and Cuemon.Extensions.IO have been updated from version 9.0.0-preview.9 to 9.0.0-preview.11. This update is likely to bring bug fixes, performance improvements, or new features from the Cuemon libraries.
However, please note:
- These are still preview versions. Consider using stable versions for production environments if available.
- It's important to verify if there are any breaking changes in these updated packages that might affect your project.
To check for potential breaking changes or new features, you can run the following command:
This script will help you locate and review any changelog or release notes files in the project, which might contain information about breaking changes or new features in the updated packages.
src/Codebelt.Extensions.AspNetCore.Text.Yaml/Bootstrapper.cs (2)
1-3: LGTM: Import statements are appropriate.The import statements are concise and relevant to the functionality of the
Bootstrapperclass.
6-7: LGTM: Class structure and declaration are well-designed.The
Bootstrapperclass is appropriately declared as internal and static, following good practices for a bootstrapper pattern.src/Codebelt.Extensions.AspNetCore.Mvc.Formatters.Text.Yaml/Codebelt.Extensions.AspNetCore.Mvc.Formatters.Text.Yaml.csproj (1)
14-14: LGTM: Package reference updated correctly.The Cuemon.AspNetCore.Mvc package has been updated from version 9.0.0-preview.9 to 9.0.0-preview.11. This change is appropriate and aligns with the project's apparent goal of staying up-to-date with the latest preview releases.
However, please note the following:
As this is a preview version, be aware that it may introduce breaking changes or unstable features. Ensure that your project remains compatible with this new version.
It's recommended to review the changelog or release notes for Cuemon.AspNetCore.Mvc 9.0.0-preview.11 to understand any new features, bug fixes, or potential breaking changes that may affect your project.
Consider running a comprehensive test suite to verify that this update doesn't introduce any regressions in your project.
To verify the impact of this change, you can run the following script:
This script will help ensure that the package update doesn't introduce any immediate issues in your project.
src/Codebelt.Extensions.AspNetCore.Text.Yaml/Codebelt.Extensions.AspNetCore.Text.Yaml.csproj (2)
14-15: Package reference updates look good. Verify changelog for any breaking changes.The package references for Cuemon.AspNetCore and Cuemon.Extensions.DependencyInjection have been updated from version 9.0.0-preview.9 to 9.0.0-preview.11. This update aligns with the project's move to support .NET 9.0.
To ensure a smooth transition, please:
- Review the changelog for these packages to check for any breaking changes or new features that may affect your code.
- Run all tests to verify compatibility with the updated packages.
- Check if there are any new features in the updated packages that could be beneficial to implement.
You can use the following command to check for any usage of deprecated APIs:
#!/bin/bash # Search for usage of potentially deprecated APIs rg -i 'obsolete|deprecated' --type cs
Line range hint
4-4: Verify the target framework changes and consider potential breaking changes.The project now targets .NET 9.0 and .NET 8.0, dropping support for earlier versions. This change may introduce breaking changes for users on older .NET versions. Additionally, .NET 9.0 is not yet officially released.
Consider the following:
- Confirm that dropping support for .NET 6.0 and 7.0 is intentional.
- Ensure that all dependencies are compatible with .NET 9.0 preview.
- Update documentation to reflect the new minimum required .NET version.
- Consider keeping support for .NET 7.0 until .NET 9.0 is officially released.
To check for any remaining references to older .NET versions in the project, run:
src/Codebelt.Extensions.YamlDotNet/Diagnostics/ExceptionDescriptorExtensions.cs (2)
23-23: LGTM: Options validation updated correctly.The change to use
Validator.ThrowIfInvalidConfiguratorwithYamlFormatterOptionsis consistent with the method signature update. This maintains proper input validation while adapting to the new options type.
20-20: Approve the signature change, but verify its impact.The change from
Action<ExceptionDescriptorOptions>toAction<YamlFormatterOptions>aligns with the PR objectives of enhancing YAML conversion capabilities. This change allows for more flexible YAML formatting options.To ensure this change doesn't break existing code, please run the following script to check for any usages of the old method signature:
If the script returns any results, those locations will need to be updated to use the new
YamlFormatterOptions.src/Codebelt.Extensions.YamlDotNet/Converters/YamlConverterExtensions.cs (2)
1-10: LGTM: Class structure and documentation are appropriate.The overall structure of the
YamlConverterExtensionsclass is well-defined. It's correctly placed in theCodebelt.Extensions.YamlDotNet.Convertersnamespace, and the class is appropriately declared as public and static. The summary documentation provides a clear description of the class's purpose.
11-16: LGTM: Method signature and documentation are well-defined.The
AddFailureConvertermethod is correctly defined as a public static extension method. The XML documentation provides clear information about the method's purpose, parameter, and return value. The method signature is appropriate, taking anICollection<YamlConverter>and returning the same type to allow for method chaining..nuget/Codebelt.Extensions.AspNetCore.Mvc.Formatters.Text.Yaml/PackageReleaseNotes.txt (2)
Line range hint
1-2: Clarify .NET 9 support and consider adding migration information
The mention of .NET 9 support might be premature, as .NET 9 is not yet officially released. Consider one of the following options:
a. Remove the mention of .NET 9 if support is not yet implemented.
b. Clarify that .NET 9 support is preliminary or in preparation for the upcoming release.Given the removal of .NET 6 support, it would be helpful to include migration information or link to a migration guide for users upgrading from .NET 6 to .NET 8.
To verify the current .NET support, run the following script:
#!/bin/bash # Check for .NET version support in project files rg --type xml "<TargetFramework(s?)>" -C 3This will help confirm which .NET versions are actually supported in the project files.
🧰 Tools
🪛 LanguageTool
[style] ~5-~5: ‘with respect to’ might be wordy. Consider a shorter alternative.
Context: ...GED Dependencies to latest and greatest with respect to TFMs
- REMOVED Support for TFM .NET 6 ...
(EN_WORDINESS_PREMIUM_WITH_RESPECT_TO)
Line range hint
1-7: Highlight breaking change: Removal of .NET 6 supportThe removal of support for .NET 6 (LTS) is a significant breaking change that should be prominently communicated to users. Consider the following suggestions:
- Add a clear "Breaking Changes" section at the top of the release notes to emphasize this change.
- Update all relevant documentation, README files, and migration guides to reflect this change.
- Consider providing migration steps for users upgrading from .NET 6 to .NET 8 or 9.
To ensure all documentation is updated, run the following script:
This will help identify any remaining references to .NET 6 support that need to be updated.
🧰 Tools
🪛 LanguageTool
[style] ~5-~5: ‘with respect to’ might be wordy. Consider a shorter alternative.
Context: ...GED Dependencies to latest and greatest with respect to TFMs
- REMOVED Support for TFM .NET 6 ...
(EN_WORDINESS_PREMIUM_WITH_RESPECT_TO)
test/Codebelt.Extensions.YamlDotNet.Tests/Converters/ExceptionDescriptorConverterTest.cs (3)
9-9: LGTM: Import statement added for naming conventions.The addition of the import statement for
YamlDotNet.Serialization.NamingConventionsis appropriate and necessary for the changes made in the test method.
Line range hint
1-67: Summary: Test improvements align with PR objectives.The changes in this file enhance the
ExceptionDescriptorConverterTest:
- Added import for naming conventions.
- Updated
YamlFormatterconfiguration to usePascalCaseNamingConvention.- Added
ExceptionDescriptorConverterwith full sensitivity details.These improvements align well with the PR objectives, particularly the use of
PascalCaseNamingConvention. The test now provides better coverage for YAML serialization with the new configuration.
32-36: LGTM: YamlFormatter configuration updated.The changes to the
YamlFormatterconfiguration are appropriate:
- Setting the naming convention to
PascalCaseNamingConvention.Instancealigns with the PR objectives.- Adding the
ExceptionDescriptorConverterwithFaultSensitivityDetails.Allenhances the test coverage.Please verify that the test assertions (starting from line 41) still pass with these changes. The serialization output might have changed due to the new naming convention and converter.
src/Codebelt.Extensions.YamlDotNet/Converters/YamlConverter.cs (1)
27-27: Request clarification on the removal ofSetPropertyNamemethod.The
SetPropertyNamemethod has been removed without any visible replacement in this file. This is a breaking change that could significantly impact existing code.Could you please provide more information on:
- The rationale behind removing this method?
- How property name handling is now managed in the YAML conversion process?
- Are there any migration steps that users of this library need to follow to adapt to this change?
To help verify the impact of this change, please run the following script:
#!/bin/bash # Search for usages of SetPropertyName method in the codebase rg "SetPropertyName\(" --type csharpThis will help us identify any remaining usages of the removed method in the codebase and ensure that all necessary updates have been made.
.nuget/Codebelt.Extensions.YamlDotNet/PackageReleaseNotes.txt (2)
16-17: Add a note about potential impacts of serialization changesThe change in YamlSerializerOptions to use camelCase naming convention and omit null values by default is a significant improvement. However, it's important to note that this change might affect existing serialization behavior. Consider adding a note about potential impacts and how users can maintain previous behavior if needed.
To verify the impact of these changes, let's search for usages of YamlSerializerOptions:
#!/bin/bash # Description: Search for usages of YamlSerializerOptions echo "Searching for YamlSerializerOptions usage:" rg --type csharp "YamlSerializerOptions" -A 5🧰 Tools
🪛 LanguageTool
[uncategorized] ~17-~17: This verb does not appear to agree with the subject. Consider using a different form.
Context: ...ng convention instead of PascalCase and omit null values by default
Version 8.4....(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
8-11: Highlight breaking changes in the release notesThe breaking changes listed here are significant and will require updates in the consuming code. It's crucial to ensure that these changes are prominently communicated to the users of the library.
To verify the impact of these breaking changes, let's search for usages of the affected classes and methods:
🧰 Tools
🪛 LanguageTool
[grammar] ~8-~8: You’ve repeated a verb. Did you mean to only write one of them?
Context: ...ort for TFM .NET 6 (LTS)
Breaking Changes
- CHANGED ToYaml method on the ExceptionDescripto...
(REPEATED_VERBS)
[uncategorized] ~10-~10: The grammatical number of this noun doesn’t look right. Consider replacing it.
Context: ... YamlFormatter parameter in all factory methods
- CHANGED YamlConverter class in the ...
(AI_EN_LECTOR_REPLACEMENT_NOUN_NUMBER)
Directory.Build.props (1)
84-84: LGTM! Verify test suite compatibility with the updated package.The update of
Codebelt.Extensions.Xunitfrom version9.0.0-preview.6to9.0.0-preview.10aligns with the overall version update mentioned in the PR title. This change likely introduces new features or bug fixes for the testing framework.To ensure this update doesn't introduce any breaking changes, please run the following script to check for any failing tests after this update:
test/Codebelt.Extensions.AspNetCore.Text.Yaml.Tests/AuthorizationResponseHandlerTest.cs (3)
17-17: LGTM: New using directive is appropriate.The addition of
using YamlDotNet.Serialization.NamingConventions;is necessary and consistent with the changes made in the test method.
35-35: LGTM: Updated YAML formatter configuration.The modification to use
PascalCaseNamingConventionin theAddYamlExceptionResponseFormattermethod is consistent with the PR objectives and ensures that the YAML output in the test uses PascalCase naming convention.
35-35: Verify test assertions for PascalCase output.The change in naming convention to PascalCase might affect the expected output in the test assertions. Please review and update the assertions if necessary to match the new PascalCase format.
✅ Verification successful
✅ Test assertions correctly use PascalCase naming convention.
All relevantAssert.Equalstatements inAuthorizationResponseHandlerTest.csalign with the new PascalCase settings.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the test assertions match the new PascalCase format # Test: Search for YAML assertions in the test file rg --type csharp -A 10 'Assert\.Equal\(' 'test/Codebelt.Extensions.AspNetCore.Text.Yaml.Tests/AuthorizationResponseHandlerTest.cs'Length of output: 1684
src/Codebelt.Extensions.YamlDotNet/Converters/ExceptionConverter.cs (1)
71-71: LGTM! Consider verifying Formatter initialization.The change to use
Formatter.Options.SetPropertyName("Type")instead of directly setting the property name is a good improvement. It centralizes the property naming logic, which can lead to more consistent naming across the application and easier maintenance.To ensure robustness, please verify that the
Formatterand itsOptionsare always properly initialized beforeWriteYamlis called. You can add a null check at the beginning of the method:if (Formatter?.Options == null) { throw new InvalidOperationException("Formatter or its Options are not initialized."); }This will help catch any potential issues early and provide a clear error message.
test/Codebelt.Extensions.YamlDotNet.Tests/Diagnostics/ExceptionDescriptorExtensionsTest.cs (1)
10-10: LGTM: Added necessary using directive.The addition of
using YamlDotNet.Serialization.NamingConventions;is appropriate and consistent with the changes made in the test methods wherePascalCaseNamingConventionis used.test/Codebelt.Extensions.YamlDotNet.Tests/YamlSerializerTest.cs (3)
13-13: LGTM: Necessary import added for PascalCaseNamingConventionThe addition of
using YamlDotNet.Serialization.NamingConventions;is appropriate and necessary for the subsequent usage ofPascalCaseNamingConvention.Instancein the test methods.
185-185: LGTM: Consistent use of PascalCaseNamingConventionThe addition of
PascalCaseNamingConvention.Instanceto the serialization settings is consistent with the changes made in other test methods. This ensures that the number format information is serialized using a consistent PascalCase naming convention.
Line range hint
1-443: Overall assessment: Changes are consistent and improve YAML serializationThe modifications made to this test file are consistent across all test methods and align well with the PR objectives. Key improvements include:
- Consistent use of
PascalCaseNamingConvention.Instancefor all serialization settings.- Updated
YamlConverterFactory.Create<DateTime>lambda to match the new method signature.- Proper import of necessary namespaces.
These changes enhance the YAML serialization capabilities and maintain consistency in naming conventions throughout the tests.
To ensure that these changes don't introduce any regressions, please run the following command to execute all tests in this file:
If all tests pass, we can be confident that the changes have not introduced any unintended side effects.
src/Codebelt.Extensions.YamlDotNet/Converters/ExceptionDescriptorConverter.cs (9)
3-3: ImportingCodebelt.Extensions.YamlDotNet.FormattersnamespaceThe addition of this namespace import is appropriate and likely required for the use of
Formatterin the class.
24-25: Enhanced configurator validation in constructorThe constructor now uses
Validator.ThrowIfInvalidConfigurator(setup, out var options);to validate thesetupaction and retrieve theoptions. This ensures that the configurator is valid before proceeding, enhancing robustness.
36-36: Consistent property naming with formatter optionsUpdated the property name to use
Formatter.Options.SetPropertyName("Error"), ensuring consistent naming conventions as per the formatter settings.
39-40: Applying formatter options to property namesProperty names "Code" and "Message" are now set using
Formatter.Options.SetPropertyName, aligning with the configured naming conventions.
43-43: Formatting "HelpLink" property name using formatter optionsThe property name "HelpLink" is now formatted with
Formatter.Options.SetPropertyName("HelpLink"), maintaining consistency.
47-47: Consistent formatting for "Failure" property nameUsing
Formatter.Options.SetPropertyName("Failure")ensures the "Failure" property name adheres to the formatter's naming conventions.
50-50: Assigning formatter to nestedExceptionConverterSetting
Formatter = Formatteron theExceptionConverterinstance ensures that the nested exception is serialized using the same formatter settings, maintaining consistency.
58-58: Consistent formatting for "Evidence" property nameThe property name "Evidence" is now written with
Formatter.Options.SetPropertyName("Evidence"), aligning it with the formatter's naming conventions.
62-63: Formatting evidence keys and values consistently
- Line 62: Property names for evidence keys are now formatted using
Formatter.Options.SetPropertyName(evidence.Key).- Line 63: Evidence values are written using
writer.WriteObject(evidence.Value, Formatter.Options), ensuring they are serialized with the correct formatter options.src/Codebelt.Extensions.AspNetCore.Text.Yaml/Formatters/ServiceCollectionExtensions.cs (1)
20-23: Verify the necessity of the static constructor and its impact on application startupThe addition of the static constructor in
ServiceCollectionExtensionsthat callsBootstrapper.Initialize()may have unintended side effects on application initialization. Static constructors run before any static member is accessed or an instance is created, which can impact startup performance and introduce hidden dependencies.Please ensure that:
- Calling
Bootstrapper.Initialize()at this point is necessary for the application's functionality.- There are no side effects that could affect the dependency injection container or service registrations.
Bootstrapper.Initialize()is thread-safe and does not introduce race conditions during application startup.src/Codebelt.Extensions.AspNetCore.Text.Yaml/Converters/YamlConverterExtensions.cs (8)
11-12: Necessary Imports AddedThe added using directives
Microsoft.AspNetCore.MvcandYamlDotNet.Coreare necessary forProblemDetailsandIEmitterrespectively.
28-29: Correct Implementation of ProblemDetails ConverterThe converters for
ProblemDetailsandIDecorator<ProblemDetails>are correctly added, utilizing theWriteProblemDetailsmethod for serialization.
33-49:WriteProblemDetailsMethod Handles Serialization AppropriatelyThe
WriteProblemDetailsmethod effectively serializes all standard properties ofProblemDetails, includingType,Title,Status,Detail, andInstance. It also correctly handles extensions by iterating overpd.Extensionsand writing non-null values.
61-86: Ensure Proper Initialization ofExceptionDescriptorOptionsWithin the lambda expression,
Validator.ThrowIfInvalidConfigurator(setup, out var options);is used to initializeoptions. Verify that whensetupis null,optionsis properly initialized with default values to prevent potentialNullReferenceExceptionwhen accessingoptions.SensitivityDetails.
79-86: Conditional Writing of Failure Details Based on Sensitivity SettingsThe block that writes the
Failureproperty is correctly conditioned onoptions.SensitivityDetails.HasFlag(FaultSensitivityDetails.Failure). TheExceptionConverteris properly configured with flags forStackTraceandDatabased on the sensitivity settings.
89-99: Correct Handling of Evidence Writing Based on SensitivityThe
Evidencesection is conditionally written based onoptions.SensitivityDetails.HasFlag(FaultSensitivityDetails.Evidence)and whethervalue.Evidence.Any()is true. The implementation correctly writes each piece of evidence to the YAML output.
101-113: Serialization of CorrelationId, RequestId, and TraceIdThe properties
CorrelationId,RequestId, andTraceIdare serialized only if they are not null or whitespace, which is appropriate to avoid unnecessary output in the YAML when these properties are unset.
117-119: Converter Addition Check Prevents Duplicate ConvertersThe check
if (!converters.Any(c => c.CanConvert(typeof(HttpExceptionDescriptor))))ensures that the converter is only added if one does not already exist forHttpExceptionDescriptor, preventing duplicate converters in the collection.src/Codebelt.Extensions.YamlDotNet/YamlConverterFactory.cs (2)
5-5: Import alias for 'YamlFormatter'The addition of the using alias for
YamlFormatterimproves code readability by avoiding fully qualified names.
65-67: Class renamed toDynamicYamlConverterFactoryRenaming the internal class to
DynamicYamlConverterFactoryenhances clarity and better reflects its purpose related to YAML conversion.test/Codebelt.Extensions.AspNetCore.Text.Yaml.Tests/ServiceCollectionExtensions.cs (1)
78-484: Ensure project targets .NET 7.0 or higher to support C# 11 featuresThe code utilizes C# 11 features such as raw string literals (triple-quoted strings) in the test assertions. Please verify that the project targets .NET 7.0 or higher by setting the correct Target Framework Moniker (TFM) in the project file. This ensures compatibility and prevents potential compilation issues.
| internal static void Initialize() | ||
| { | ||
| if (!_initialized) | ||
| { | ||
| lock (PadLock) | ||
| { | ||
| if (!_initialized) | ||
| { | ||
| _initialized = true; | ||
| YamlFormatterOptions.DefaultConverters += list => | ||
| { | ||
| list.AddProblemDetailsConverter(); | ||
| }; | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
LGTM: Thread-safe initialization is well-implemented.
The Initialize method correctly uses the double-checked locking pattern to ensure thread-safe, single initialization. The logic for adding the ProblemDetailsConverter is concise and effective.
Consider extracting the converter addition logic into a separate private method for improved readability:
internal static void Initialize()
{
if (!_initialized)
{
- lock (PadLock)
+ lock (_padLock)
{
if (!_initialized)
{
_initialized = true;
- YamlFormatterOptions.DefaultConverters += list =>
- {
- list.AddProblemDetailsConverter();
- };
+ AddDefaultConverters();
}
}
}
}
+private static void AddDefaultConverters()
+{
+ YamlFormatterOptions.DefaultConverters += list =>
+ {
+ list.AddProblemDetailsConverter();
+ };
+}This refactoring improves readability and separates concerns, making the code more maintainable.
Committable suggestion was skipped due to low confidence.
| { | ||
| converters.Add(YamlConverterFactory.Create<Failure>((writer, failure, formatter) => | ||
| { | ||
| new ExceptionConverter(formatter.Options.SensitivityDetails.HasFlag(FaultSensitivityDetails.StackTrace), formatter.Options.SensitivityDetails.HasFlag(FaultSensitivityDetails.Data)) | ||
| { | ||
| Formatter = formatter | ||
| }.WriteYaml(writer, failure.GetUnderlyingException()); | ||
| })); | ||
| return converters; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring for improved readability and add documentation.
The implementation of AddFailureConverter is functionally correct, but could be improved:
- The lambda expression passed to
YamlConverterFactory.Createis complex. Consider extracting it into a separate method for better readability. - The use of
formatter.Options.SensitivityDetailsin creating theExceptionConverteris not documented. It would be helpful to add a comment explaining the significance of these flags.
Here's a suggested refactor:
public static ICollection<YamlConverter> AddFailureConverter(this ICollection<YamlConverter> converters)
{
converters.Add(YamlConverterFactory.Create<Failure>(WriteFailureToYaml));
return converters;
}
private static void WriteFailureToYaml(YamlWriter writer, Failure failure, YamlFormatter formatter)
{
// Create ExceptionConverter with sensitivity options
// SensitivityDetails.StackTrace: Include stack trace in the output
// SensitivityDetails.Data: Include exception data in the output
var converter = new ExceptionConverter(
includeStackTrace: formatter.Options.SensitivityDetails.HasFlag(FaultSensitivityDetails.StackTrace),
includeData: formatter.Options.SensitivityDetails.HasFlag(FaultSensitivityDetails.Data))
{
Formatter = formatter
};
converter.WriteYaml(writer, failure.GetUnderlyingException());
}This refactoring improves readability by separating the converter logic into its own method and adds comments explaining the use of SensitivityDetails.
| WhiteSpaceIndentation = 2; | ||
| UseAliases = false; | ||
| NamingConvention = NullNamingConvention.Instance; | ||
| NamingConvention = CamelCaseNamingConvention.Instance; |
There was a problem hiding this comment.
Breaking change: Default naming convention changed to camelCase
The default value for NamingConvention has been changed from NullNamingConvention.Instance to CamelCaseNamingConvention.Instance. This is a breaking change that will affect the default serialization behavior.
Implications:
- Improved alignment with common JSON naming conventions (camelCase).
- Potential breaking of existing integrations that rely on the previous default behavior.
- May require updates to deserialization logic in consuming code.
Consider the following actions:
- Document this change prominently in release notes.
- Provide migration guidelines for users updating from previous versions.
- Consider adding a compatibility mode or configuration option to preserve the old behavior if backwards compatibility is a concern.
| IndentSequences = true; | ||
| TextWidth = int.MaxValue; | ||
| ValuesHandling = DefaultValuesHandling.Preserve; | ||
| ValuesHandling = DefaultValuesHandling.OmitNull; |
There was a problem hiding this comment.
Breaking change: Default values handling changed to omit null values
The default value for ValuesHandling has been changed from DefaultValuesHandling.Preserve to DefaultValuesHandling.OmitNull. This is a breaking change that will affect the default serialization behavior.
Implications:
- More compact YAML output by default, as null values will be omitted.
- Potential breaking of existing integrations that rely on null values being preserved.
- May require updates to deserialization logic in consuming code, especially if null value presence is significant.
- Aligns better with common serialization practices in other formats like JSON.
Consider the following actions:
- Document this change prominently in release notes.
- Provide migration guidelines for users updating from previous versions.
- Ensure that this change is consistent with the project's overall serialization strategy across different formats.
- Consider adding a compatibility mode or configuration option to preserve the old behavior if backwards compatibility is a concern.
| services.TryAddSingleton(provider => | ||
| { | ||
| var options = provider.GetService<IOptions<YamlFormatterOptions>>().Value; | ||
| var faultDescriptorOptions = provider.GetRequiredService<IOptions<FaultDescriptorOptions>>().Value; |
There was a problem hiding this comment.
Ensure 'FaultDescriptorOptions' is registered in the service collection
The code retrieves FaultDescriptorOptions using provider.GetRequiredService<IOptions<FaultDescriptorOptions>>().Value;. If FaultDescriptorOptions has not been registered, this will throw an exception at runtime.
Please verify that FaultDescriptorOptions is properly registered in the service collection before this point. If it is not registered, consider adding a registration similar to:
services.AddOptions<FaultDescriptorOptions>();| public static ICollection<YamlConverter> AddHttpExceptionDescriptorConverter(this ICollection<YamlConverter> converters, Action<ExceptionDescriptorOptions> setup = null) | ||
| { | ||
| var options = Patterns.Configure(setup); | ||
| var converter = YamlConverterFactory.Create<HttpExceptionDescriptor>(type => type == typeof(HttpExceptionDescriptor), (writer, value) => | ||
| var converter = YamlConverterFactory.Create<HttpExceptionDescriptor>(type => type == typeof(HttpExceptionDescriptor), (writer, value, formatter) => | ||
| { | ||
| Validator.ThrowIfInvalidConfigurator(setup, out var options); | ||
|
|
||
| writer.WriteStartObject(); | ||
| writer.WritePropertyName(options.SetPropertyName("Error")); | ||
| writer.WritePropertyName(formatter.Options.SetPropertyName("Error")); | ||
|
|
||
| writer.WriteStartObject(); | ||
| writer.WriteString(options.SetPropertyName("Status"), value.StatusCode.ToString(CultureInfo.InvariantCulture)); | ||
| writer.WriteString(options.SetPropertyName("Code"), value.Code); | ||
| writer.WriteString(options.SetPropertyName("Message"), value.Message); | ||
| if (value.Instance != null) | ||
| { | ||
| writer.WritePropertyName(formatter.Options.SetPropertyName("Instance")); | ||
| writer.WriteValue(value.Instance.OriginalString); | ||
| } | ||
| writer.WriteString(formatter.Options.SetPropertyName("Status"), value.StatusCode.ToString(CultureInfo.InvariantCulture)); | ||
| writer.WriteString(formatter.Options.SetPropertyName("Code"), value.Code); | ||
| writer.WriteString(formatter.Options.SetPropertyName("Message"), value.Message); | ||
| if (value.HelpLink != null) | ||
| { | ||
| writer.WriteString(options.SetPropertyName("HelpLink"), value.HelpLink.OriginalString); | ||
| writer.WriteString(formatter.Options.SetPropertyName("HelpLink"), value.HelpLink.OriginalString); | ||
| } | ||
| if (options.SensitivityDetails.HasFlag(FaultSensitivityDetails.Failure)) | ||
| { | ||
| writer.WritePropertyName(options.SetPropertyName("Failure")); | ||
| writer.WritePropertyName(formatter.Options.SetPropertyName("Failure")); | ||
| new ExceptionConverter(options.SensitivityDetails.HasFlag(FaultSensitivityDetails.StackTrace), options.SensitivityDetails.HasFlag(FaultSensitivityDetails.Data)) | ||
| { | ||
| FormatterOptions = options | ||
| Formatter = formatter | ||
| }.WriteYaml(writer, value.Failure); | ||
| } | ||
| writer.WriteEndObject(); | ||
|
|
||
| if (options.SensitivityDetails.HasFlag(FaultSensitivityDetails.Evidence) && value.Evidence.Any()) | ||
| { | ||
| writer.WritePropertyName(options.SetPropertyName("Evidence")); | ||
| writer.WritePropertyName(formatter.Options.SetPropertyName("Evidence")); | ||
| writer.WriteStartObject(); | ||
| foreach (var evidence in value.Evidence) | ||
| { | ||
| writer.WritePropertyName(options.SetPropertyName(evidence.Key)); | ||
| writer.WriteObject(evidence.Value, options); | ||
| writer.WritePropertyName(formatter.Options.SetPropertyName(evidence.Key)); | ||
| writer.WriteObject(evidence.Value, formatter.Options); | ||
| } | ||
| writer.WriteEndObject(); | ||
| } | ||
|
|
||
| if (!string.IsNullOrWhiteSpace(value.CorrelationId)) | ||
| { | ||
| writer.WriteString(options.SetPropertyName("CorrelationId"), value.CorrelationId); | ||
| writer.WriteString(formatter.Options.SetPropertyName(nameof(value.CorrelationId)), value.CorrelationId); | ||
| } | ||
| if (!string.IsNullOrWhiteSpace(value.RequestId)) | ||
| { | ||
| writer.WriteString(options.SetPropertyName("RequestId"), value.RequestId); | ||
| writer.WriteString(formatter.Options.SetPropertyName(nameof(value.RequestId)), value.RequestId); | ||
| } | ||
|
|
||
| if (!string.IsNullOrWhiteSpace(value.TraceId)) | ||
| { | ||
| writer.WriteString(formatter.Options.SetPropertyName(nameof(value.TraceId)), value.TraceId); | ||
| } | ||
|
|
||
| writer.WriteEndObject(); | ||
| }); | ||
| converter.FormatterOptions = options; | ||
|
|
||
| if (!converters.Any(c => c.CanConvert(typeof(HttpExceptionDescriptor)))) { converters.Add(converter); } | ||
| return converters; | ||
| } |
There was a problem hiding this comment.
Breaking Change: Updated AddHttpExceptionDescriptorConverter Method Signature
The AddHttpExceptionDescriptorConverter method now accepts Action<ExceptionDescriptorOptions> instead of Action<YamlFormatterOptions> for the setup parameter. This change alters how consumers configure the converter and represents a breaking change. Ensure this change is documented, and dependent code is updated to pass the correct configuration action.
| public static YamlConverter Create<T>(Action<IEmitter, T, YamlFormatter> writer = null, Func<IParser, Type, YamlFormatter, T> reader = null) | ||
| { | ||
| return new DynamicConvertFactory<T>(typeof(T).IsAssignableFrom, writer, reader); | ||
| return new DynamicYamlConverterFactory<T>(typeof(T).IsAssignableFrom, writer, reader); |
There was a problem hiding this comment.
Breaking Change: Method signatures updated with YamlFormatter parameter
The public Create methods now include an additional YamlFormatter parameter in the writer and reader delegates. This breaking change affects users implementing custom converters. Ensure that this change is clearly documented in the release notes and migration guides to help users update their code.
Also applies to: 35-37, 47-49, 59-61
|
|
||
| namespace Codebelt.Extensions.AspNetCore.Text.Yaml | ||
| { | ||
| public class ServiceCollectionExtensions : Test |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider renaming the test class for clarity
The class ServiceCollectionExtensions is a test class, but its name may cause confusion as it suggests it contains extension methods for IServiceCollection. To improve clarity and follow naming conventions, consider renaming it to ServiceCollectionExtensionsTests to indicate that it contains unit tests.
| [InlineData(FaultSensitivityDetails.FailureWithStackTrace)] | ||
| [InlineData(FaultSensitivityDetails.Failure)] | ||
| [InlineData(FaultSensitivityDetails.None)] | ||
| public async Task AddYamlExceptionResponseFormatter_ShouldCaptureException_RenderAsExceptionDescriptor_UsingYaml(FaultSensitivityDetails sensitivity) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Review test method naming conventions for readability
The test method names are quite lengthy, which may affect readability and maintainability. Consider adopting a naming convention that is descriptive yet concise. For example, you might rename:
AddYamlExceptionResponseFormatter_ShouldCaptureException_RenderAsExceptionDescriptor_UsingYamltoCaptureException_AsExceptionDescriptor_YamlFormatAddYamlExceptionResponseFormatter_ShouldCaptureException_RenderAsProblemDetails_UsingYamltoCaptureException_AsProblemDetails_YamlFormat
Also applies to: 262-262
| throw new NotFoundException("Main exception - look out for inner!", e); | ||
| } | ||
|
|
||
| await next(context); |
There was a problem hiding this comment.
Remove unreachable code after exception is thrown
After throwing the NotFoundException on line 59, the control flow exits the delegate, and the await next(context); statement on line 62 is unreachable. Removing this line will clean up the code and prevent potential confusion.
Apply this diff to remove the unreachable code:
Committable suggestion was skipped due to low confidence.
1 participant
PR Classification
New feature and breaking changes to enhance YAML conversion capabilities and update dependencies.
PR Summary
This PR introduces new YAML conversion features, breaking changes to method signatures, and updates to dependencies and naming conventions.
YamlConverterFactory.cs: AddedYamlFormatterparameter and renamed internal classes,YamlSerializerOptions.cs: Changed default naming convention to camelCase and omitted null values,ServiceCollectionExtensions.cs: Added new test methods for YAML exception response formatter,AuthorizationResponseHandlerTest.cs: Updated to usePascalCaseNamingConvention,testenvironments.jsonto a new version.Summary by CodeRabbit
AddProblemDetailsConverterandAddFailureConvertermethods for enhanced YAML serialization.ToYamlmethod to support new parameter types.