Enable recommended analyzers; use invariant culture throughout #640
Conversation
📝 WalkthroughWalkthroughProject-wide refactors emphasize culture-invariant formatting, stricter exception types, and static helper conversions. Dispose methods in core classes become virtual. A new QRCodeGenerator.EciMode enum is added. Extensions gain HexColorToByteArray and AttributeUsage on StringValueAttribute. Build/test configs adjust analyzer settings and warnings; API approval baselines updated. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
.editorconfig(1 hunks)Directory.Build.props(1 hunks)QRCoder.Xaml/QRCoder.Xaml.csproj(1 hunks)QRCoder/ASCIIQRCode.cs(1 hunks)QRCoder/ArtQRCode.cs(5 hunks)QRCoder/Base64QRCode.cs(1 hunks)QRCoder/BitmapByteQRCode.cs(2 hunks)QRCoder/Extensions/StringExtensions.cs(1 hunks)QRCoder/Extensions/StringValueAttribute.cs(1 hunks)QRCoder/PayloadGenerator.cs(2 hunks)QRCoder/PayloadGenerator/BezahlCode.cs(10 hunks)QRCoder/PayloadGenerator/BitcoinLikeCryptoCurrencyAddress.cs(1 hunks)QRCoder/PayloadGenerator/CalendarEvent.cs(1 hunks)QRCoder/PayloadGenerator/ContactData.cs(2 hunks)QRCoder/PayloadGenerator/Girocode.cs(2 hunks)QRCoder/PayloadGenerator/MoneroTransaction.cs(1 hunks)QRCoder/PayloadGenerator/OneTimePassword.cs(4 hunks)QRCoder/PayloadGenerator/RussiaPaymentOrder.cs(3 hunks)QRCoder/PayloadGenerator/SlovenianUpnQr.cs(4 hunks)QRCoder/PayloadGenerator/SwissQrCode.cs(3 hunks)QRCoder/PdfByteQRCode.cs(1 hunks)QRCoder/PostscriptQRCode.cs(2 hunks)QRCoder/QRCode.cs(1 hunks)QRCoder/QRCodeData.cs(2 hunks)QRCoder/QRCodeGenerator.cs(6 hunks)QRCoder/QRCodeGenerator/Point.cs(1 hunks)QRCoder/QRCodeGenerator/Polynom.cs(2 hunks)QRCoder/QRCoder.csproj(1 hunks)QRCoder/SvgQRCode.cs(6 hunks)QRCoderTests/PayloadGeneratorTests/BezahlCodeTests.cs(14 hunks)
🧰 Additional context used
🧬 Code graph analysis (16)
QRCoder/PayloadGenerator/OneTimePassword.cs (1)
QRCoder/Extensions/StringExtensions.cs (1)
IsNullOrWhiteSpace(13-31)
QRCoder/Extensions/StringValueAttribute.cs (1)
QRCoder/Attributes/NotNullWhenAttribute.cs (1)
AttributeUsage(8-25)
QRCoder/PayloadGenerator/Girocode.cs (3)
QRCoder/PayloadGenerator.cs (2)
IsValidBic(69-70)IsValidBic(78-83)QRCoder/PayloadGenerator/BezahlCode.cs (1)
ToString(247-328)QRCoder/PayloadGenerator/SwissQrCode.cs (3)
ToString(293-294)ToString(480-490)ToString(544-598)
QRCoder/PdfByteQRCode.cs (1)
QRCoder/Extensions/StringExtensions.cs (1)
HexColorToByteArray(38-51)
QRCoder/PayloadGenerator/ContactData.cs (4)
QRCoder/PayloadGenerator/BezahlCode.cs (1)
ToString(247-328)QRCoder/PayloadGenerator/BitcoinLikeCryptoCurrencyAddress.cs (1)
ToString(46-65)QRCoder/PayloadGenerator/Girocode.cs (1)
ToString(75-95)QRCoder/PayloadGenerator/MoneroTransaction.cs (1)
ToString(40-48)
QRCoder/QRCodeData.cs (1)
QRCoder/AbstractQRCode.cs (1)
Dispose(42-46)
QRCoder/PayloadGenerator/MoneroTransaction.cs (3)
QRCoder/PayloadGenerator/BezahlCode.cs (1)
ToString(247-328)QRCoder/PayloadGenerator/BitcoinLikeCryptoCurrencyAddress.cs (1)
ToString(46-65)QRCoder/PayloadGenerator/SwissQrCode.cs (3)
ToString(293-294)ToString(480-490)ToString(544-598)
QRCoder/PayloadGenerator/SwissQrCode.cs (3)
QRCoder/PayloadGenerator/BezahlCode.cs (1)
ToString(247-328)QRCoder/PayloadGenerator/BitcoinLikeCryptoCurrencyAddress.cs (1)
ToString(46-65)QRCoder/PayloadGenerator/MoneroTransaction.cs (1)
ToString(40-48)
QRCoder/PayloadGenerator.cs (1)
QRCoder/Extensions/StringExtensions.cs (1)
ToString(55-56)
QRCoder/BitmapByteQRCode.cs (1)
QRCoder/Extensions/StringExtensions.cs (1)
HexColorToByteArray(38-51)
QRCoder/PayloadGenerator/BezahlCode.cs (4)
QRCoder/PayloadGenerator.cs (2)
IsValidBic(69-70)IsValidBic(78-83)QRCoder/PayloadGenerator/BitcoinLikeCryptoCurrencyAddress.cs (1)
ToString(46-65)QRCoder/PayloadGenerator/ContactData.cs (1)
ToString(114-249)QRCoder/PayloadGenerator/Girocode.cs (1)
ToString(75-95)
QRCoder/PayloadGenerator/RussiaPaymentOrder.cs (4)
QRCoder/PayloadGenerator/BezahlCode.cs (1)
ToString(247-328)QRCoder/PayloadGenerator/BitcoinLikeCryptoCurrencyAddress.cs (1)
ToString(46-65)QRCoder/PayloadGenerator/Girocode.cs (1)
ToString(75-95)QRCoder/PayloadGenerator/SwissQrCode.cs (3)
ToString(293-294)ToString(480-490)ToString(544-598)
QRCoder/PayloadGenerator/CalendarEvent.cs (4)
QRCoder/PayloadGenerator/BezahlCode.cs (1)
ToString(247-328)QRCoder/PayloadGenerator/BitcoinLikeCryptoCurrencyAddress.cs (1)
ToString(46-65)QRCoder/PayloadGenerator/ContactData.cs (1)
ToString(114-249)QRCoder/PayloadGenerator/MoneroTransaction.cs (1)
ToString(40-48)
QRCoderTests/PayloadGeneratorTests/BezahlCodeTests.cs (1)
QRCoder/PayloadGenerator/BezahlCode.cs (10)
PayloadGenerator(3-1124)BezahlCode(8-1123)BezahlCode(32-34)BezahlCode(53-55)BezahlCode(76-78)BezahlCode(106-244)BezahlCodeException(1095-1122)BezahlCodeException(1100-1102)BezahlCodeException(1108-1111)BezahlCodeException(1118-1121)
QRCoder/QRCodeGenerator.cs (2)
QRCoder/QRCodeData.cs (6)
QRCodeData(8-247)QRCodeData(19-26)QRCodeData(33-40)QRCodeData(47-49)QRCodeData(56-122)Dispose(222-227)QRCoder/AbstractQRCode.cs (1)
Dispose(42-46)
QRCoder/PayloadGenerator/SlovenianUpnQr.cs (3)
QRCoder/PayloadGenerator/BezahlCode.cs (1)
ToString(247-328)QRCoder/PayloadGenerator/BitcoinLikeCryptoCurrencyAddress.cs (1)
ToString(46-65)QRCoder/PayloadGenerator/MoneroTransaction.cs (1)
ToString(40-48)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: additional-tests
- GitHub Check: build
🔇 Additional comments (36)
QRCoder/QRCodeGenerator/Polynom.cs (2)
38-39: LGTM! Correct exception type for parameter validation.Replacing
IndexOutOfRangeExceptionwithArgumentOutOfRangeExceptionis the appropriate choice for validating method parameters. The use ofnameof(index)is good practice for maintainability.
54-55: LGTM! Good refactoring with proper attributes.The refactoring to use a centralized
ThrowArgumentOutOfRangeException()helper method reduces duplication and improves maintainability. TheStackTraceHiddenattribute is a nice touch that keeps stack traces clean by hiding the helper method from diagnostics.Note: The helper uses the string literal
"index"rather thannameof(index)since indexers have implicit parameters without named identifiers—this is acceptable and follows .NET conventions.Also applies to: 60-61, 69-69
QRCoder/QRCodeGenerator/Point.cs (1)
45-54: LGTM! Proper equality and hash code implementation.The Equals and GetHashCode overrides follow best practices for structs implementing IEquatable. The conditional compilation appropriately uses HashCode.Combine on modern runtimes while providing a valid rotate-XOR fallback for older targets.
.editorconfig (1)
266-266: LGTM!Disabling IDE0022 (expression body for methods) is a deliberate configuration choice that relaxes code style enforcement. This aligns with the PR's analyzer configuration objectives.
QRCoderTests/PayloadGeneratorTests/BezahlCodeTests.cs (1)
14-16: Correct warning suppression codes.The change from CS0612 to CS0618 is correct. CS0618 is for obsolete members with a message (which matches the
[Obsolete]attributes on the deprecatedAuthorityTypeenum members in BezahlCode.cs), while CS0612 is for obsolete members without a message.Also applies to: 33-35, 52-54, 75-77, 248-250, 264-266, 312-314, 327-329, 374-376, 388-390, 455-457, 469-471, 484-486, 619-621
QRCoder/Base64QRCode.cs (1)
144-144: LGTM!Converting
BitmapToBase64to a static method is appropriate since it doesn't access any instance state. This follows best practices for helper methods.QRCoder.Xaml/QRCoder.Xaml.csproj (1)
8-8: LGTM!Enabling the recommended analysis mode aligns with the PR's objective of improving code quality through static analysis.
QRCoder/ASCIIQRCode.cs (1)
34-34: LGTM!Using
ArgumentOutOfRangeExceptionwith the parameter name is the correct exception type for parameter validation, making the error more specific and actionable.QRCoder/PayloadGenerator/ContactData.cs (1)
139-139: LGTM!Adding
CultureInfo.InvariantCultureto date formatting ensures consistent payload generation regardless of the user's locale. This is essential for vCard/MeCard compatibility.Also applies to: 235-235
QRCoder/PayloadGenerator/CalendarEvent.cs (1)
53-54: LGTM!Adding
CultureInfo.InvariantCultureensures culture-invariant date/time formatting for iCalendar events, which is critical for cross-platform compatibility and standard compliance.QRCoder/PayloadGenerator/BitcoinLikeCryptoCurrencyAddress.cs (1)
64-64: LGTM!Using
ToLowerInvariant()instead ofToLower()ensures culture-invariant URI generation for cryptocurrency addresses. This prevents locale-specific casing issues (e.g., Turkish locale's 'I' → 'ı' problem) that could break protocol compliance.QRCoder/PayloadGenerator/MoneroTransaction.cs (1)
45-45: LGTM! Culture-invariant formatting applied correctly.The change to use
CultureInfo.InvariantCultureensures the decimal separator is consistently a period (.) in the URI, regardless of the system's culture settings.QRCoder/Extensions/StringValueAttribute.cs (1)
10-10: LGTM! Appropriate usage restriction.Adding
AttributeUsage(AttributeTargets.Field)correctly restricts this attribute to field declarations only, which aligns with howGetStringValueretrieves it via reflection (GetField/GetRuntimeField).QRCoder/QRCoder.csproj (1)
8-8: LGTM! Analyzer configuration enabled.Adding
AnalysisModeset toRecommendedaligns with the PR objective of enabling recommended analyzers for improved code quality.QRCoder/ArtQRCode.cs (1)
137-161: LGTM! Appropriate staticization of helper methods.Converting
MakeDotPixel,IsPartOfQuietZone,IsPartOfFinderPattern, andResizeto static methods is correct since they don't access instance state.Also applies to: 171-178, 189-199, 207-231
QRCoder/PayloadGenerator/SwissQrCode.cs (3)
55-55: LGTM! Culture-invariant amount validation.Using
CultureInfo.InvariantCultureensures the length calculation is consistent regardless of locale-specific decimal separators.
278-278: LGTM! Ordinal comparison for IBAN prefix.Using
StringComparison.Ordinalfor checking IBAN prefixes ("CH" and "LI") ensures culture-insensitive validation.
594-594: LGTM! Ordinal comparison for line break check.Using
StringComparison.Ordinalfor checking the trailing line break ensures culture-insensitive string comparison.Directory.Build.props (1)
41-41: LGTM! Analyzer suppression configured.Adding CA1707 to
NoWarnsuppresses identifier naming warnings. This is a deliberate configuration choice with no functional impact.QRCoder/SvgQRCode.cs (2)
155-157: LGTM! Pragma suppressions handle analyzer limitations.The CA1305 pragmas suppress warnings where string interpolation doesn't directly support format providers, while the underlying
CleanSvgValmethod already usesCultureInfo.InvariantCulture(line 249). This approach is appropriate.Also applies to: 184-186, 197-199
219-220: LGTM! Appropriate staticization of helper methods.Converting
IsBlockedByLogo,GetLogoAttributes,CleanSvgVal, andGetMimeTypeto static methods is correct since they don't access instance state.Also applies to: 222-235, 248-249, 393-398
QRCoder/PayloadGenerator/OneTimePassword.cs (2)
55-55: LGTM! Cleaner property declaration.Removing the explicit
= nullinitialization is appropriate sinceint?already defaults tonull. This makes the code cleaner without changing behavior.
156-156: LGTM! More specific exception types.Changing from generic
ExceptiontoInvalidOperationExceptionis appropriate for validation errors where the object is in an invalid state.Also applies to: 179-179, 190-190, 199-199
QRCoder/BitmapByteQRCode.cs (2)
44-44: LGTM!The refactor to use the shared extension method eliminates code duplication and follows good DRY principles.
143-143: LGTM!Converting this utility method to static is appropriate since it doesn't access instance state. This improves clarity and allows for potential inlining optimizations.
QRCoder/PdfByteQRCode.cs (1)
51-52: LGTM!The refactor to use the shared extension method is consistent with similar changes in BitmapByteQRCode.cs and eliminates code duplication.
QRCoder/QRCodeGenerator.cs (7)
27-29: LGTM!The CA1822 pragma suppressions are appropriate for maintaining API backward compatibility. These instance methods delegate to static methods but are kept non-static to preserve the public API surface for existing consumers.
Also applies to: 39-41, 55-57, 67-69
203-203: LGTM!
InvalidOperationExceptionis semantically appropriate for this scenario where the required encoding is not supported for the requested Micro QR code version. This represents an invalid operation rather than a general exception.
1379-1382: LGTM!Making
Disposevirtual and addingGC.SuppressFinalize(this)follows the proper IDisposable pattern. This allows derived classes to override the disposal behavior correctly and is consistent with similar changes in QRCodeData.cs.
27-29: LGTM! Pragmas appropriately suppress CA1822 for API compatibility.The
CreateQrCodeinstance methods delegate to staticGenerateQrCodemethods, so they could technically be static. However, keeping them as instance methods preserves the public API. The pragmas are properly paired and document this intentional design choice.Also applies to: 39-41, 55-57, 67-69
203-203: LGTM! InvalidOperationException is semantically appropriate.Using
InvalidOperationExceptionwhen an encoding is not supported for a given version is appropriate, as it indicates the operation cannot be completed due to the current state. This correctly distinguishes fromDataTooLongExceptionthrown on line 204 for size-related issues.
1379-1383: LGTM! Dispose pattern correctly implements standard best practices.Making
Dispose()virtual enables derived classes to extend cleanup logic, and addingGC.SuppressFinalize(this)follows the standard dispose pattern to suppress finalization after disposal. These changes align with similar updates inQRCodeData.cs.
1015-1040: LGTM! Culture-invariant parsing follows best practices.Using
CultureInfo.InvariantCulturewhen parsing numeric strings ensures consistent behavior regardless of the user's locale settings. This aligns with the PR objective of adopting invariant culture throughout the codebase.QRCoder/Extensions/StringExtensions.cs (2)
38-51: LGTM!The implementation correctly parses hex color strings to byte arrays using invariant culture. The method appropriately delegates input validation to
byte.Parse, which will throw meaningful exceptions for invalid input (null strings, invalid hex characters, or odd-length strings).
53-57: LGTM!This compatibility shim for NETSTANDARD1_3 is appropriate. The CultureInfo parameter is intentionally unused (indicated by the
_parameter name) sincechar.ToString()doesn't perform culture-sensitive operations.QRCoder/QRCode.cs (1)
155-155: LGTM! Making the method static appropriately reflects stateless design.
CreateRoundedRectanglePathdoesn't access any instance members, so marking itstaticis appropriate and follows analyzer recommendations. Since the method isinternal, this is not a breaking change, and the call site at line 137 continues to work without modification.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
QRCoder/QRCodeGenerator/EciMode.cs (1)
7-36: Clarify ECI header behavior forDefaultvsIso8859_1.
Default(0): omits the ECI header and relies on the QR spec’s default ISO-8859-1 assumption.Iso8859_1(3): emits an explicit ECI header (value 3) to select ISO-8859-1 encoding.Update the XML summaries in EciMode.cs to clearly state these differences.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
QRCoder/PayloadGenerator/BezahlCode.cs(12 hunks)QRCoder/PayloadGenerator/Girocode.cs(4 hunks)QRCoder/PayloadGenerator/RussiaPaymentOrder.cs(5 hunks)QRCoder/PayloadGenerator/SMS.cs(2 hunks)QRCoder/QRCodeGenerator/EciMode.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- QRCoder/PayloadGenerator/RussiaPaymentOrder.cs
- QRCoder/PayloadGenerator/BezahlCode.cs
- QRCoder/PayloadGenerator/Girocode.cs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: additional-tests
🔇 Additional comments (2)
QRCoder/PayloadGenerator/SMS.cs (1)
50-69: Targeted suppression for CA1707 looks good.The pragma is scoped just to the enum that needs the underscore, keeping the analyzer happy without touching the public API surface.
QRCoder/QRCodeGenerator/EciMode.cs (1)
1-2: LGTM! Pragma appropriately suppresses underscore warnings.The pragma correctly suppresses CA1707 warnings for the enum member names that use underscores (e.g.,
Iso8859_1,Iso8859_2). This naming convention preserves the standard character set nomenclature and improves readability.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
QRCoder/PayloadGenerator/SlovenianUpnQr.cs (1)
151-151: Consider removing redundantToUpperInvariant().
_codeis already converted to uppercase in the constructor (line 92), making this call redundant. While harmless, removing it would avoid an unnecessary string operation.Apply this diff if desired:
- _sb.Append(_code.ToUpperInvariant()).Append('\n'); + _sb.Append(_code).Append('\n');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
QRCoder/PayloadGenerator/SlovenianUpnQr.cs(4 hunks)QRCoder/PostscriptQRCode.cs(2 hunks)QRCoder/QRCodeGenerator/Polynom.cs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- QRCoder/QRCodeGenerator/Polynom.cs
🧰 Additional context used
🧬 Code graph analysis (1)
QRCoder/PayloadGenerator/SlovenianUpnQr.cs (3)
QRCoder/PayloadGenerator/Girocode.cs (1)
ToString(75-95)QRCoder/PayloadGenerator/BezahlCode.cs (1)
ToString(247-328)QRCoder/PayloadGenerator/BitcoinLikeCryptoCurrencyAddress.cs (1)
ToString(46-65)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: additional-tests
- GitHub Check: build
🔇 Additional comments (7)
QRCoder/PostscriptQRCode.cs (3)
103-106: LGTM! Culture-invariant formatting ensures consistent PostScript output.The addition of
CultureInfo.InvariantCultureto the header formatting ensures that the PostScript output remains consistent across different locales, which is critical for generating valid PostScript files.
107-111: LGTM! Culture-invariant formatting for numeric values.Adding
CultureInfo.InvariantCultureto the PS_FUNCTIONS formatting ensures that RGB color values and the module count are formatted consistently regardless of the user's locale settings.
131-131: LGTM! Appropriate conversion to static method.Converting
CleanSvgValto a static method is correct since it doesn't rely on any instance state, and the culture-invariant formatting is already in place.QRCoder/PayloadGenerator/SlovenianUpnQr.cs (4)
48-49: LGTM on static conversion.Converting
LimitLengthto static is appropriate since it doesn't access instance state.
92-92: LGTM on invariant culture usage in constructor.Using
ToUpperInvariant()andCultureInfo.InvariantCultureensures consistent formatting across different locales, which aligns with the PR objectives.Also applies to: 94-94, 99-99
109-113: LGTM on static conversion and invariant culture.Converting
FormatAmountto static is appropriate, and usingCultureInfo.InvariantCultureensures consistent numeric formatting.
154-154: LGTM on invariant culture usage in ToString.The use of
ToUpperInvariant()for IBAN andCultureInfo.InvariantCulturefor checksum formatting is correct and consistent with the PR's standardization goals.Also applies to: 159-159
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
QRCoderDemo/QRCoderDemo.csproj (1)
8-8: Use canonical culture casingPlease prefer
en-USso the neutral language matches the canonical BCP 47 casing and stays consistent with other culture tags.QRCoderApiTests/ApiApprovalTests.cs (1)
1-2: Justify or refactor CA1861 suppression
Inline array initializers inApiApprovalTests.cs(AllowNamespacePrefixes and ExcludeAttributes) will trigger CA1861; keep the pragma or extract these arrays intostatic readonlyfields to satisfy the analyzer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
Directory.Build.props(1 hunks)QRCoder/ArtQRCode.cs(5 hunks)QRCoderApiTests/ApiApprovalTests.cs(2 hunks)QRCoderBenchmarks/QRCoderBenchmarks.csproj(1 hunks)QRCoderConsole/Program.cs(3 hunks)QRCoderDemo/QRCoderDemo.csproj(1 hunks)QRCoderTests/ArtQRCodeRendererTests.cs(1 hunks)QRCoderTests/Helpers/HelperFunctions.cs(0 hunks)QRCoderTests/Helpers/ShouldMatchApprovedBinaryExtensions.cs(1 hunks)QRCoderTests/PayloadGeneratorTests/BezahlCodeTests.cs(20 hunks)QRCoderTests/QRCoderTests.csproj(2 hunks)QRCoderTests/QRGeneratorTests.cs(1 hunks)
💤 Files with no reviewable changes (1)
- QRCoderTests/Helpers/HelperFunctions.cs
✅ Files skipped from review due to trivial changes (1)
- QRCoderTests/QRGeneratorTests.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- QRCoderTests/ArtQRCodeRendererTests.cs
🧰 Additional context used
🧬 Code graph analysis (1)
QRCoderTests/PayloadGeneratorTests/BezahlCodeTests.cs (1)
QRCoder/PayloadGenerator/BezahlCode.cs (7)
PayloadGenerator(3-1126)BezahlCode(8-1125)BezahlCode(32-34)BezahlCode(53-55)BezahlCode(76-78)BezahlCode(106-244)ToString(247-328)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Test .NET 6.0 Windows
- GitHub Check: Test .NET 6.0
- GitHub Check: Test .NET 5.0 Windows
- GitHub Check: Test .NET Core 2.1
- GitHub Check: Test .NET Core 3.1
- GitHub Check: Test .NET 5.0
- GitHub Check: additional-tests
🔇 Additional comments (11)
QRCoderBenchmarks/QRCoderBenchmarks.csproj (1)
8-8: CA1822 suppression makes sense here.BenchmarkDotNet expects benchmark methods to stay instance members even when they could be static; suppressing CA1822 locally avoids churn without harming coverage. Looks good.
QRCoderTests/Helpers/ShouldMatchApprovedBinaryExtensions.cs (1)
60-72: LGTM! Exception specificity improved.Replacing generic
ExceptionwithShouldAssertExceptionacross these validation paths appropriately distinguishes test infrastructure failures from application-level errors, improving test diagnostics and aligning with the Shouldly framework conventions.QRCoderTests/PayloadGeneratorTests/BezahlCodeTests.cs (2)
14-16: LGTM: Pragma warning updates align with enhanced obsolete attributes.The change from CS0612 to CS0618 correctly reflects the updated
AuthorityTypeenum members, which now include guidance messages in their[Obsolete]attributes directing users to SEPA-compliant alternatives. The tests appropriately suppress these warnings when intentionally exercising backward compatibility.Also applies to: 33-35, 52-54, 75-77, 248-250, 264-266, 312-314, 327-329, 374-376, 388-390, 455-457, 469-471, 484-486, 619-621
19-19: LGTM: Explicit invariant culture ensures consistent test behavior.Adding
CultureInfo.InvariantCulturetoDateTime.Now.ToStringcalls aligns with the PR objective and prevents culture-dependent test failures. This ensures the test assertions match the production code's culture-invariant date formatting inBezahlCode.ToString().Also applies to: 38-38, 57-57, 80-80, 96-96, 115-115, 136-136, 159-159, 253-253, 269-269, 285-285, 301-301, 317-317
Directory.Build.props (1)
43-43: LGTM - Enables recommended analyzers.This change aligns with the PR objectives and improves code quality by enabling the recommended set of .NET code analyzers during builds.
QRCoderApiTests/ApiApprovalTests.cs (1)
66-66: LGTM! Appropriate use of Ordinal comparison for technical identifiers.Using
StringComparison.Ordinalis correct here for comparing Target Framework Moniker (TFM) directory names. TFMs are technical identifiers (e.g., "net6.0", "netstandard2.0"), not culture-sensitive user text, so Ordinal comparison provides:
- Correct semantics (binary comparison of technical strings)
- Better performance (no culture-specific rules)
Note that this is case-sensitive, but MSBuild's TFM directory names should match the project file's TFM values exactly in case.
QRCoderConsole/Program.cs (4)
35-35: LGTM! Static method call is correct.The conversion from instance to static method access is correctly implemented here.
148-148: LGTM! Static method access is correct.The static call to
GetImageFormatis properly implemented with a safe non-null argument from the enum'sToString().
225-228: LGTM! Static class and method are well-implemented.The conversion to a static class is appropriate for these utility methods. The
GetECCLevelimplementation is concise, handles invalid input gracefully by returning a default value, and aligns with the PR's objective of converting helpers to static.
233-241: LGTM! Excellent use of invariant culture.The
GetImageFormatmethod correctly implements culture-invariant string comparison usingToLowerInvariant(), which directly aligns with the PR's objective of using invariant culture throughout the codebase. The switch expression is clear, covers common image formats appropriately, and provides a sensible default.QRCoder/ArtQRCode.cs (1)
137-137: LGTM! Appropriate static conversions.Converting these helper methods to static is correct since they don't access instance state. This follows best practices and likely addresses analyzer recommendations.
Also applies to: 171-171, 189-189, 207-207
gfoidl
left a comment
gfoidl
left a comment
There was a problem hiding this comment.
We can look at the suggestions later.
Anyway I'll do a optimization pass accross the code base when I have a bit more time.
Shane32
added
ci
Summary by CodeRabbit