V9.0.1/package maintenance #2
Conversation
WalkthroughThis pull request introduces updates across multiple files, focusing on upgrading to .NET 9.0, modifying package dependencies, and adjusting project configurations. Key changes include updating the Nginx base image, enhancing CI/CD pipeline configurations, and centralizing package version management. Additionally, copyright information is updated, and new tests are added to validate YAML serialization. The changes reflect a comprehensive effort to modernize the project and streamline dependency management. Changes
Possibly related PRs
Poem
🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
tooling/gse/gse.csproj (1)
Update gse.csproj to target net9.0
The gse.csproj file is explicitly targeting net8.0, while the PR objectives require updating to net9.0. This project should be updated to align with other projects in the solution that already support net9.0.
- Update
<TargetFrameworks>net8.0</TargetFrameworks>to<TargetFrameworks>net9.0</TargetFrameworks>in tooling/gse/gse.csproj🔗 Analysis chain
Line range hint
4-4: Update target framework to net9.0.The PR objectives indicate updating target frameworks to net9.0, but this project is still targeting net8.0.
Run this script to check target framework consistency across projects:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check target framework consistency across projects # Find all .csproj files and check their TargetFramework(s) fd -e csproj -x grep -l "TargetFramework" {} \; -exec echo "File: {}" \; -exec grep -A 1 "TargetFramework" {} \;Length of output: 348
Script:
#!/bin/bash # Find all .csproj files and check their framework targets find . -name "*.csproj" -type f -exec sh -c ' echo "=== $1 ===" grep -A 1 "<TargetFramework" "$1" ' sh {} \;Length of output: 430
Script:
#!/bin/bash # Find all .csproj files and check their framework targets with broader search find . -name "*.csproj" -type f -exec sh -c ' echo "=== $1 ===" echo "Content:" cat "$1" echo -e "\n" ' sh {} \;Length of output: 63170
Script:
#!/bin/bash # Find and show content of Directory.Build.props files find . -name "Directory.Build.props" -type f -exec sh -c ' echo "=== $1 ===" cat "$1" ' sh {} \;Length of output: 4290
🧹 Nitpick comments (4)
test/Codebelt.Extensions.Globalization.Tests/YamlSerializerTest.cs (1)
35-174: Consider adding more test cases for DateTimeFormat serialization.The test only covers the Danish culture. Consider adding test cases for:
- Cultures with different calendar systems
- Cultures with AM/PM designators
.github/workflows/pipelines.yml (1)
29-29: Add spaces after commas in framework list.The framework matrix update looks good, but fix the YAML formatting:
- framework: [net9.0,net8.0,netstandard2.0] + framework: [net9.0, net8.0, netstandard2.0]🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 29-29: too few spaces after comma
(commas)
[warning] 29-29: too few spaces after comma
(commas)
CHANGELOG.md (2)
7-9: Fix grammar and improve conciseness in the note.The note contains grammar issues and can be more concise.
-> [!NOTE] -> Changelog entries prior to version 9.0.1 was migrated from previous versions of Cuemon.Extensions.Globalization. +> [!NOTE] +> Changelog entries before version 9.0.1 were migrated from previous versions of Cuemon.Extensions.Globalization.🧰 Tools
🪛 LanguageTool
[style] ~8-~8: ‘prior to’ might be wordy. Consider a shorter alternative.
Context: ...er.[!NOTE]
Changelog entries prior to version 9.0.1 was migrated from previou...(EN_WORDINESS_PREMIUM_PRIOR_TO)
[uncategorized] ~8-~8: The verb “was” doesn’t seem to fit in this context, “were” is probably more formally correct.
Context: ...hangelog entries prior to version 9.0.1 was migrated from previous versions of Cuem...(AI_HYDRA_LEO_CPT_WAS_WERE)
10-13: Enhance the changelog entry with specific details.The current description is quite brief. Consider adding more details about the key changes mentioned in the PR objectives.
## [9.0.1] - 2025-01-29 -This is a service update that primarily focuses on package dependencies and minor improvements. +### Changed + +- Updated target frameworks to .NET 9.0 +- Updated package dependencies to their latest versions +- Updated Docker images and configurations +- Refreshed copyright information for 2025
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
.docfx/Dockerfile.docfx(1 hunks).docfx/docfx.json(2 hunks).github/workflows/pipelines.yml(1 hunks).nuget/Codebelt.Extensions.Globalization/PackageReleaseNotes.txt(1 hunks)CHANGELOG.md(1 hunks)Directory.Build.props(3 hunks)Directory.Build.targets(1 hunks)Directory.Packages.props(1 hunks)LICENSE(1 hunks)src/Codebelt.Extensions.Globalization/Codebelt.Extensions.Globalization.csproj(1 hunks)test/Codebelt.Extensions.Globalization.Tests/YamlSerializerTest.cs(1 hunks)testenvironments.json(1 hunks)tooling/gse/gse.csproj(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- LICENSE
- .docfx/Dockerfile.docfx
- testenvironments.json
🧰 Additional context used
🪛 LanguageTool
.nuget/Codebelt.Extensions.Globalization/PackageReleaseNotes.txt
[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)
CHANGELOG.md
[style] ~8-~8: ‘prior to’ might be wordy. Consider a shorter alternative.
Context: ...er.
[!NOTE]
Changelog entries prior to version 9.0.1 was migrated from previou...
(EN_WORDINESS_PREMIUM_PRIOR_TO)
[uncategorized] ~8-~8: The verb “was” doesn’t seem to fit in this context, “were” is probably more formally correct.
Context: ...hangelog entries prior to version 9.0.1 was migrated from previous versions of Cuem...
(AI_HYDRA_LEO_CPT_WAS_WERE)
🪛 YAMLlint (1.35.1)
.github/workflows/pipelines.yml
[warning] 29-29: too few spaces after comma
(commas)
[warning] 29-29: too few spaces after comma
(commas)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: 🧪 Test (windows-2022, Release)
- GitHub Check: 🧪 Test (windows-2022, Debug)
- GitHub Check: 🧪 Test (ubuntu-22.04, Release)
- GitHub Check: 📦 Pack (Release)
- GitHub Check: 🧪 Test (ubuntu-22.04, Debug)
- GitHub Check: 📦 Pack (Debug)
🔇 Additional comments (10)
Directory.Packages.props (2)
2-4: LGTM! Central package version management is properly configured.The setup enables consistent package versioning across the project.
6-14: Verify package version compatibility with .NET 9.The package versions look recent, but since we're targeting .NET 9, let's verify their compatibility.
✅ Verification successful
Testing packages are compatible with .NET 9
The selected package versions are all very recent (late 2024) and specifically target .NET 9 compatibility:
- Microsoft.NET.Test.Sdk 17.12.0 is the recommended version for .NET 9
- xunit 2.9.x series was released with .NET 9 support
- All other packages are framework-agnostic testing tools
Run the test suite to confirm everything works as expected.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check package compatibility with .NET 9 # Test: Query NuGet API for each package's framework compatibility for package in "Microsoft.NET.Test.Sdk" "MinVer" "coverlet.collector" "coverlet.msbuild" "xunit" "xunit.runner.console" "xunit.runner.visualstudio"; do echo "Checking $package..." curl -s "https://api.nuget.org/v3/registration5-gz-semver2/$package/index.json" | jq -r '.items[0].items[0].catalogEntry | select(.version == "17.12.0" or .version == "6.0.0" or .version == "6.0.4" or .version == "2.9.3" or .version == "3.0.1") | {version: .version, frameworks: .dependencyGroups[].targetFramework}' doneLength of output: 3907
Script:
#!/bin/bash # Description: Check package compatibility with .NET 9 using different API endpoint # Test: Query NuGet API for each package's framework compatibility for package in "Microsoft.NET.Test.Sdk" "MinVer" "coverlet.collector" "coverlet.msbuild" "xunit" "xunit.runner.console" "xunit.runner.visualstudio"; do echo "Checking $package..." version=$(grep "\"$package\"" Directory.Packages.props | grep -o 'Version="[^"]*"' | cut -d'"' -f2) curl -s "https://api.nuget.org/v3/registration5-gz-semver2/${package,,}/$version.json" | jq -r '.catalogEntry.dependencyGroups[].targetFramework' doneLength of output: 4094
Script:
#!/bin/bash # Get framework compatibility for Microsoft.NET.Test.Sdk curl -s "https://api.nuget.org/v3/registration5-semver2/microsoft.net.test.sdk/17.12.0.json" | \ jq -r '.catalogEntry.dependencyGroups[].targetFramework' # Also check the package listing to ensure it's the latest version curl -s "https://api.nuget.org/v3/registration5-semver2/microsoft.net.test.sdk/index.json" | \ jq -r '.items[0].upper'Length of output: 382
Directory.Build.targets (1)
17-18: LGTM! Improved CI integration using GitHub Actions variables.The change properly handles versioning in both CI and local environments.
✅ Verification successful
Verified: Build number migration is complete and correct
The change from BUILD_BUILDNUMBER to GITHUB_RUN_NUMBER is the only required update in the build configuration files. The implementation properly handles both CI and local builds, with good integration into the MinVer-based versioning system.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the version number format in assemblies # Test: Search for any other usage of BUILD_BUILDNUMBER that might need updating rg -l 'BUILD_BUILDNUMBER' --type-add 'props:*.props' --type-add 'targets:*.targets' -tprops -ttargetsLength of output: 101
Script:
#!/bin/bash # Try case-insensitive search with content rg -i 'buildnumber|build.?number' --type-add 'props:*.props' --type-add 'targets:*.targets' -tprops -ttargets # Show the complete Directory.Build.targets cat Directory.Build.targets # Look for version-related properties rg -i 'version|assemblyversion|fileversion' --type-add 'props:*.props' --type-add 'targets:*.targets' -tprops -ttargetsLength of output: 2426
Directory.Build.props (2)
18-19: LGTM! Framework and metadata updates are properly configured.The changes correctly:
- Add net9.0 to target frameworks
- Update copyright year
- Configure version tag prefix
Also applies to: 38-38
46-46: LGTM! Package references updated for central version management.Package references have been correctly updated to use central version management.
Also applies to: 72-84
test/Codebelt.Extensions.Globalization.Tests/YamlSerializerTest.cs (1)
436-444: LGTM! Platform-specific handling is properly implemented.The code correctly handles differences between .NET versions and platforms.
tooling/gse/gse.csproj (1)
22-22: LGTM! Package reference follows best practices.Removing explicit version from the package reference is good practice as it allows for centralized version management through Directory.Packages.props.
.docfx/docfx.json (1)
15-15: LGTM! Configuration updates are consistent.The changes appropriately:
- Update the target framework to net9.0
- Update the copyright year to include 2025
Also applies to: 48-48
.nuget/Codebelt.Extensions.Globalization/PackageReleaseNotes.txt (1)
1-6: Verify the major version bump implications.The version change from 8.4.0 to 9.0.1 is significant. Please ensure:
- Breaking changes are documented
- The removal of .NET 6 support is communicated to users
- The version follows semantic versioning principles
Run this script to check for potential breaking changes:
✅ Verification successful
Major version bump is justified and properly documented
The version change from 8.4.0 to 9.0.1 follows semantic versioning principles:
- Breaking change: Removal of .NET 6 (LTS) support
- Framework compatibility is clearly communicated in release notes
- No public API changes detected
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Look for breaking changes in the codebase # Search for removed public APIs, changed interfaces, etc. echo "Checking for removed public APIs..." ast-grep --pattern 'public $_' echo "Checking for interface changes..." ast-grep --pattern 'interface $_' echo "Checking for enum changes..." ast-grep --pattern 'enum $_'Length of output: 7161
Script:
#!/bin/bash # Check previous version's framework support and dependencies echo "Checking previous version..." fd -e csproj . -x cat {} echo "Checking git history for removed files..." git log --name-status HEAD^..HEAD echo "Checking package history..." cat .nuget/Codebelt.Extensions.Globalization/PackageReleaseNotes.txtLength of output: 65358
🧰 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)
src/Codebelt.Extensions.Globalization/Codebelt.Extensions.Globalization.csproj (1)
1203-1203: LGTM! Package reference follows best practices.Removing explicit version from the package reference is good practice as it allows for centralized version management through Directory.Packages.props.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/scorecard.yml (1)
4-5: Consider adjusting the schedule to off-peak hours.The current schedule (17:45 UTC on Tuesdays) might coincide with peak development hours. Consider running this during off-peak hours to minimize resource contention.
schedule: - - cron: '45 17 * * 2' + - cron: '45 03 * * 2'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/scorecard.yml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: 🧪 Test (windows-2022, Release)
- GitHub Check: 🧪 Test (windows-2022, Debug)
🔇 Additional comments (2)
.github/workflows/scorecard.yml (2)
11-18: Well-structured job configuration with appropriate permissions!The job configuration follows security best practices:
- Uses latest Ubuntu runner
- Implements principle of least privilege with specific write permissions
19-42: Well-implemented security analysis workflow!The implementation includes all necessary components for effective security analysis:
- Secure credential handling
- Appropriate SARIF output format
- Proper artifact management
- Integration with GitHub code scanning
| uses: actions/checkout@v4 | ||
| with: |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance security by using SHA pinning for actions.
While using major version tags (v3, v4) provides some stability, using commit SHAs provides better security by preventing supply chain attacks.
- uses: actions/checkout@v4
+ uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1
- uses: ossf/scorecard-action@v2.4.0
+ uses: ossf/scorecard-action@483ef80d5c11fa7c3087c19d0a311648c6893975 # v2.4.0
- uses: actions/upload-artifact@v4
+ uses: actions/upload-artifact@c7d193f32edcb7bfad88892161225aeda64e9392 # v4.0.0
- uses: github/codeql-action/upload-sarif@v3
+ uses: github/codeql-action/upload-sarif@407ffafae6a767df3e0230c3df91b6443ae8df75 # v3.22.11Also applies to: 26-27, 33-34, 40-41
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
test/Codebelt.Extensions.Globalization.Tests/YamlSerializerTest.cs (5)
19-19: Update the comment to reflect .NET 9 compatibility.The comment references .NET 6+, but since this PR upgrades to .NET 9, consider updating it to be more specific about the version requirements.
- private static readonly CultureInfo DanishCulture = new CultureInfo("da-DK").UseNationalLanguageSupport(); // from .NET6+ this is needed for both Windows and Linux; at least from pipeline (worked locally for Windows without Merge ...) + private static readonly CultureInfo DanishCulture = new CultureInfo("da-DK").UseNationalLanguageSupport(); // Required for both Windows and Linux in .NET 9+ pipeline environments
28-37: Improve test method readability and structure.The test method could benefit from:
- Better variable naming than
sut2,sut3,sut4- Separation of concerns between setup, execution, and assertion phases
- var sut2 = DanishCulture; - var sut3 = YamlFormatter.SerializeObject(sut2.DateTimeFormat, o => + // Arrange + var culture = DanishCulture; + var serializerOptions = new YamlFormatterOptions + { + Settings = + { + NamingConvention = PascalCaseNamingConvention.Instance, + ScalarStyle = ScalarStyle.Plain, + IndentSequences = false, + FormatProvider = culture + } + }; + serializerOptions.Settings.Converters.Add( + YamlConverterFactory.Create<DateTime>((writer, dt, _) => + writer.WriteValue(dt.ToString(culture)))); + + // Act + var serializedYaml = YamlFormatter.SerializeObject(culture.DateTimeFormat, serializerOptions); + var encodedResult = serializedYaml.ToEncodedString();
148-161: Extract platform-specific logic into a helper method.The platform-specific code handling different .NET versions could be moved to a separate method to improve readability and maintainability.
+ private string GetExpectedTwoDigitYearMax(string template) + { #if NET8_0_OR_GREATER + return string.Format(template, "2049"); #elif NET48_OR_GREATER + if (RuntimeInformation.IsOSPlatform(OSPlatform.Linux)) + { + template = StringReplacePair.ReplaceAll(template, + "gregoriansk kalender", "dansk (Danmark)", + StringComparison.Ordinal); + return string.Format(template, "2029"); + } + template = StringReplacePair.ReplaceAll(template, + "gregoriansk", "Gregoriansk", + StringComparison.Ordinal); + return string.Format(template, "2049"); #endif + }
169-177: Consider validating actual number formatting behavior.The test only verifies the YAML structure but doesn't validate that the serialized format actually produces correct number formatting when used.
+ // Arrange + var culture = DanishCulture; + var testNumber = 1234567.89; + + // Act var sut3 = YamlFormatter.SerializeObject(sut2.NumberFormat, o => { o.Settings.ScalarStyle = ScalarStyle.DoubleQuoted; o.Settings.IndentSequences = false; o.Settings.FormatProvider = DanishCulture; o.Settings.NamingConvention = PascalCaseNamingConvention.Instance; }); var sut4 = sut3.ToEncodedString(); + + // Additional validation + var deserializedFormat = YamlFormatter.DeserializeObject<NumberFormatInfo>(sut4); + var formattedNumber = testNumber.ToString("N", deserializedFormat); + + // Assert + Assert.Equal("1.234.567,89", formattedNumber);
228-234: Add validation for culture-specific behavior.The test only verifies the YAML structure but doesn't validate that the serialized culture actually produces correct formatting when used.
var sut2 = DanishCulture; + var testDate = new DateTime(2025, 1, 15); + var testNumber = 1234567.89; + var sut3 = YamlFormatter.SerializeObject(sut2, o => { o.Settings.IndentSequences = false; o.Settings.NamingConvention = PascalCaseNamingConvention.Instance; o.Settings.Converters.Add(YamlConverterFactory.Create<DateTime>((writer, dt, _) => writer.WriteValue(dt.ToString(DanishCulture)))); }); + + // Validate culture-specific behavior + var deserializedCulture = YamlFormatter.DeserializeObject<CultureInfo>(sut3); + Assert.Equal("15. januar 2025", testDate.ToString("d. MMMM yyyy", deserializedCulture)); + Assert.Equal("1.234.567,89", testNumber.ToString("N", deserializedCulture));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/Codebelt.Extensions.Globalization.Tests/YamlSerializerTest.cs(1 hunks)
🔇 Additional comments (1)
test/Codebelt.Extensions.Globalization.Tests/YamlSerializerTest.cs (1)
21-23: Consider improving error handling in the constructor.The constructor should handle potential initialization failures and utilize the output parameter for logging.
public YamlSerializerTest(ITestOutputHelper output) : base(output) { + output.WriteLine($"Test initialization started at: {DateTime.UtcNow}"); + try + { + output.WriteLine($"Assembly version: {typeof(CultureInfo).Assembly.GetName().Version}"); + var culture = new CultureInfo("da-DK").UseNationalLanguageSupport(); + // Verify culture initialization + if (culture.Name != "da-DK") + { + throw new InvalidOperationException($"Culture initialization failed. Expected: da-DK, Got: {culture.Name}"); + } + } + catch (Exception ex) + { + output.WriteLine($"Test initialization failed: {ex}"); + throw; // Rethrow to fail fast + } }
| sut4.RemoveRange(sut4.FindIndex(s => s.StartsWith("CompareInfo")), 6); | ||
| sut4.RemoveRange(sut4.FindIndex(s => s.StartsWith("CultureTypes")), 1); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace manual string manipulation with a more robust approach.
The current approach of removing ranges based on string matching could be fragile if the YAML structure changes.
- sut4.RemoveRange(sut4.FindIndex(s => s.StartsWith("CompareInfo")), 6);
- sut4.RemoveRange(sut4.FindIndex(s => s.StartsWith("CultureTypes")), 1);
+ // Define sections to exclude
+ var sectionsToExclude = new Dictionary<string, int>
+ {
+ ["CompareInfo"] = 6,
+ ["CultureTypes"] = 1
+ };
+
+ // Remove sections safely
+ foreach (var section in sectionsToExclude)
+ {
+ var index = sut4.FindIndex(s => s.StartsWith(section.Key));
+ if (index != -1)
+ {
+ sut4.RemoveRange(index, Math.Min(section.Value, sut4.Count - index));
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| sut4.RemoveRange(sut4.FindIndex(s => s.StartsWith("CompareInfo")), 6); | |
| sut4.RemoveRange(sut4.FindIndex(s => s.StartsWith("CultureTypes")), 1); | |
| // Define sections to exclude | |
| var sectionsToExclude = new Dictionary<string, int> | |
| { | |
| ["CompareInfo"] = 6, | |
| ["CultureTypes"] = 1 | |
| }; | |
| // Remove sections safely | |
| foreach (var section in sectionsToExclude) | |
| { | |
| var index = sut4.FindIndex(s => s.StartsWith(section.Key)); | |
| if (index != -1) | |
| { | |
| sut4.RemoveRange(index, Math.Min(section.Value, sut4.Count - index)); | |
| } | |
| } |
|
This pull request includes several updates to the project, focusing on upgrading dependencies, updating target frameworks, and making various adjustments to configuration files. The most important changes are summarized below:
Dependency and Framework Updates:
.docfx/Dockerfile.docfxtonginx:1.27.3-alpine..docfx/docfx.jsontonet9.0.Directory.Build.propsandDirectory.Packages.propsto includenet9.0. [1] [2]Directory.Packages.propsto the latest versions.testenvironments.jsonto supportnet9.0.Configuration and Metadata Adjustments:
.docfx/docfx.jsonto reflect the new copyright year and organization.CHANGELOG.md.LICENSEto include the year 2025.Build and CI Improvements:
MinVerTagPrefixproperty inDirectory.Build.propsfor versioning.FileVersionproperty inDirectory.Build.targetsto useGITHUB_RUN_NUMBER.Summary by CodeRabbit
Release Notes v9.0.1
New Features
Improvements
Maintenance
Breaking Changes