-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Prep for v2; target .NET Standard 2.0, .NET Standard 2.1 and .NET 6.0 #685
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis PR represents a major version bump (1.6.1 to 2.0.0) that consolidates target framework support, removes old conditional compilation paths, and refactors internal logic. Changes include narrowing target frameworks to netstandard2.0, netstandard2.1, and net6.0; eliminating NET35, NET40, and NET5.0 support; removing preprocessor directives (HAS_SPAN, SYSTEM_DRAWING, NETSTANDARD1_3); making System.Drawing usage unconditional; and significantly refactoring components like PdfByteQRCode and QRCodeGenerator. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes The changes span multiple file categories with heterogeneous modifications: straightforward preprocessor removal (low complexity, repetitive pattern), refactored logic paths with semantic changes (moderate-to-high complexity), full rewrites of critical components (PdfByteQRCode), significant public API removals, and strategic framework consolidation requiring careful verification. While some changes follow consistent patterns (removing guards), the breadth, target framework changes, and substantial internal refactoring of generation/encoding logic demand thorough review of each cohort's correctness and potential behavioral impacts. Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
Removed branch specification for pull request trigger.
…yload handling - Implemented abstract class AbstractQRCode and derived classes: ArtQRCode, AsciiQRCode, Base64QRCode, BitmapByteQRCode, PdfByteQRCode, PngByteQRCode, PostscriptQRCode, SvgQRCode, and QRCode. - Added helper classes for each QR code type to facilitate QR code generation. - Introduced PayloadGenerator with various payload types including BezahlCode, BitcoinAddress, CalendarEvent, ContactData, Geolocation, Girocode, and more. - Defined enums for various configurations and options within the QR code generation process. - Included exception handling classes for managing errors related to data size and specific payload types. - Established a comprehensive structure for QR code data management with QRCodeData and QRCodeGenerator classes.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Note: I still plan to issue another v1 release from the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
QRCoder/PdfByteQRCode.cs (2)
234-234: PDF numbers must not use scientific notation; adjust float formatting."G7" may emit exponents; PDF real numbers disallow exponent format. Use a fixed‑point format.
- private static string ToStr(float value) => value.ToString("G7", CultureInfo.InvariantCulture); + private static string ToStr(float value) => value.ToString("0.########", CultureInfo.InvariantCulture);Also safe for pdfMediaSize and scale already using ToStr(float).
Also applies to: 48-49, 102-106
1-1: Add explicit System.IO and System.Collections.Generic usings for netstandard2.0/2.1 compatibility.ImplicitUsings is enabled in Directory.Build.props with System.Text and System.Globalization, but this feature only works with .NET 6+. For netstandard2.0/2.1 targets (which the QRCoder library supports), ImplicitUsings have no effect. The file references
MemoryStreamandStreamWriter(System.IO) starting at line 57, andList<long>(System.Collections.Generic), neither of which are covered by the global usings. Add explicit local usings for these namespaces to ensure compilation on all target frameworks:using System.IO; using System.Collections.Generic; using static QRCoder.QRCodeGenerator;
🧹 Nitpick comments (7)
QRCoderTests/PayloadGeneratorTests/RussiaPaymentOrderTests.cs (1)
38-40: Encoding provider registration mirrors the previous test.This registration is correct and necessary for the KOI8-R encoding test. While the registration code is duplicated from the previous test, this is acceptable since
Encoding.RegisterProvideris idempotent. If you add more encoding tests in the future, consider moving this registration to a class constructor or test fixture setup method.QRCoder/Extensions/StreamExtensions.cs (1)
11-14: Consider using ArrayPool to avoid allocation.The call to
bytes.ToArray()allocates a new array, which defeats the zero-allocation benefit of usingReadOnlySpan<byte>. Since the PR adds a dependency onSystem.Memoryfor .NET Standard 2.0, consider usingArrayPool<byte>.Sharedto eliminate this allocation.Apply this diff to use ArrayPool:
+using System.Buffers; +using System.IO; + namespace QRCoder; internal static class StreamExtensions { public static void Write(this Stream stream, ReadOnlySpan<byte> bytes) { - stream.Write(bytes.ToArray(), 0, bytes.Length); + var array = ArrayPool<byte>.Shared.Rent(bytes.Length); + try + { + bytes.CopyTo(array); + stream.Write(array, 0, bytes.Length); + } + finally + { + ArrayPool<byte>.Shared.Return(array); + } } }QRCoder/PdfByteQRCode.cs (2)
120-121: Compute stream /Length in bytes, not characters.Safer to use the encoder’s byte count to future‑proof against non‑ASCII content.
- var content = "q\r\n" + ... + "Q"; + var content = "q\r\n" + ... + "Q"; + var contentLength = Encoding.ASCII.GetByteCount(content); ... - "<< /Length " + ToStr(content.Length) + " >>\r\n" + + "<< /Length " + ToStr(contentLength) + " >>\r\n" +Also applies to: 101-113
41-44: Deprecate the unused jpgQuality parameter (or remove in v3).It’s documented as obsolete; mark the overload [Obsolete] to guide callers without breaking now.
-[Obsolete("jpgQuality parameter is ignored and will be removed in a future major version.")] public byte[] GetGraphic(int pixelsPerModule, string darkColorHtmlHex, string lightColorHtmlHex, int dpi = 150, long jpgQuality = 85)If you prefer not to warn on the entire overload, keep docs as is.
QRCoder/QRCodeGenerator/ByteDataSegment.cs (1)
216-229: Ensure pooled buffers are returned even on exceptions.If encoding or BitArray writes throw,
bufferFromPoolis leaked. Wrap rent/use/return in try/finally.#if !NETSTANDARD2_0 // We can use stackalloc for small arrays to prevent heap allocations const int MAX_STACK_SIZE_IN_BYTES = 512; int count = targetEncoding.GetByteCount(plainText); byte[]? bufferFromPool = null; - Span<byte> codeBytes = (count <= MAX_STACK_SIZE_IN_BYTES) - ? (stackalloc byte[MAX_STACK_SIZE_IN_BYTES]) - : (bufferFromPool = ArrayPool<byte>.Shared.Rent(count)); - codeBytes = codeBytes.Slice(0, count); - targetEncoding.GetBytes(plainText, codeBytes); + Span<byte> codeBytes = (count <= MAX_STACK_SIZE_IN_BYTES) + ? (stackalloc byte[count]) + : (bufferFromPool = ArrayPool<byte>.Shared.Rent(count)); + try + { + targetEncoding.GetBytes(plainText, codeBytes); + // Write the data to the BitArray + if (includeUtf8BOM) + { + DecToBin(0xEF, 8, bitArray, offset); + DecToBin(0xBB, 8, bitArray, offset + 8); + DecToBin(0xBF, 8, bitArray, offset + 16); + offset += 24; + } + CopyToBitArray(codeBytes, bitArray, offset); + offset += (int)((uint)count * 8); + } + finally + { + if (bufferFromPool != null) + ArrayPool<byte>.Shared.Return(bufferFromPool); + } #else byte[] codeBytes = targetEncoding.GetBytes(plainText); @@ -#if !NETSTANDARD2_0 - if (bufferFromPool != null) - ArrayPool<byte>.Shared.Return(bufferFromPool); -#endif +#if NETSTANDARD2_0 + // Write the data to the BitArray (NETSTANDARD2_0 path) + if (includeUtf8BOM) + { + DecToBin(0xEF, 8, bitArray, offset); + DecToBin(0xBB, 8, bitArray, offset + 8); + DecToBin(0xBF, 8, bitArray, offset + 16); + offset += 24; + } + CopyToBitArray(codeBytes, bitArray, offset); + offset += (int)((uint)codeBytes.Length * 8); +#endifAlso applies to: 243-246
QRCoder/Extensions/StringExtensions.cs (1)
72-77: Broaden polyfill guard to match platforms lacking string.StartsWith(char).Use the established guard so the polyfill appears on frameworks without the API, not only netstandard2.0.
Based on learnings
-#if NETSTANDARD2_0 +#if !NETCOREAPP2_0_OR_GREATER && !NETSTANDARD2_1_OR_GREATER public static bool StartsWith(this string target, char value) { return target.Length > 0 && target[0] == value; } #endifQRCoder/SvgQRCode.cs (1)
331-336: Use InvariantCulture explicitly in hex parsing for clarity.Parsing with NumberStyles.HexNumber ignores culture; passing InvariantCulture makes this explicit.
- if (int.TryParse(colorHex.AsSpan(7, 2), NumberStyles.HexNumber, null, out int alpha)) + if (int.TryParse(colorHex.AsSpan(7, 2), NumberStyles.HexNumber, CultureInfo.InvariantCulture, out int alpha)) @@ - if (int.TryParse(colorHex.AsSpan(4, 1), NumberStyles.HexNumber, null, out int alpha)) + if (int.TryParse(colorHex.AsSpan(4, 1), NumberStyles.HexNumber, CultureInfo.InvariantCulture, out int alpha))Also applies to: 345-350
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (49)
.github/workflows/wf-build-release-ci.yml(1 hunks).github/workflows/wf-build-test.yml(0 hunks).github/workflows/wf-verify-formatting.yml(0 hunks)Directory.Build.props(2 hunks)QRCoder.Xaml/QRCoder.Xaml.csproj(2 hunks)QRCoder/ASCIIQRCode.cs(0 hunks)QRCoder/ArtQRCode.cs(0 hunks)QRCoder/Attributes/NotNullWhenAttribute.cs(1 hunks)QRCoder/Attributes/StackTraceHiddenAttribute.cs(1 hunks)QRCoder/Attributes/SupportedOSPlatformAttribute.cs(1 hunks)QRCoder/Base64QRCode.cs(0 hunks)QRCoder/Extensions/BitArrayExtensions.cs(1 hunks)QRCoder/Extensions/StreamExtensions.cs(1 hunks)QRCoder/Extensions/StringExtensions.cs(5 hunks)QRCoder/Extensions/StringValueAttribute.cs(0 hunks)QRCoder/PayloadGenerator.cs(0 hunks)QRCoder/PayloadGenerator/OneTimePassword.cs(2 hunks)QRCoder/PayloadGenerator/RussiaPaymentOrder.cs(0 hunks)QRCoder/PdfByteQRCode.cs(1 hunks)QRCoder/PngByteQRCode.cs(0 hunks)QRCoder/PostscriptQRCode.cs(0 hunks)QRCoder/QRCode.cs(0 hunks)QRCoder/QRCodeData.cs(0 hunks)QRCoder/QRCodeGenerator.cs(3 hunks)QRCoder/QRCodeGenerator/AlphanumericEncoder.cs(0 hunks)QRCoder/QRCodeGenerator/ByteDataSegment.cs(2 hunks)QRCoder/QRCodeGenerator/CodewordBlock.cs(0 hunks)QRCoder/QRCodeGenerator/GaloisField.cs(0 hunks)QRCoder/QRCodeGenerator/NumericDataSegment.cs(2 hunks)QRCoder/QRCodeGenerator/Polynom.cs(0 hunks)QRCoder/QRCoder.csproj(2 hunks)QRCoder/SvgQRCode.cs(2 hunks)QRCoderApiTests/net60/QRCoder.approved.txt(0 hunks)QRCoderApiTests/netstandard13/QRCoder.approved.txt(0 hunks)QRCoderApiTests/netstandard20+netstandard21/QRCoder.approved.txt(0 hunks)QRCoderConsole/Program.cs(2 hunks)QRCoderConsole/QRCoderConsole.csproj(2 hunks)QRCoderDemo/QRCoderDemo.csproj(1 hunks)QRCoderTests/ArtQRCodeRendererTests.cs(0 hunks)QRCoderTests/Base64QRCodeRendererTests.cs(0 hunks)QRCoderTests/PayloadGeneratorTests/BitcoinCashAddressTests.cs(0 hunks)QRCoderTests/PayloadGeneratorTests/LitecoinAddressTests.cs(0 hunks)QRCoderTests/PayloadGeneratorTests/RussiaPaymentOrderTests.cs(2 hunks)QRCoderTests/QRCodeRendererTests.cs(0 hunks)QRCoderTests/QRCoderTests.csproj(2 hunks)QRCoderTests/QRGeneratorTests.cs(0 hunks)QRCoderTests/SvgQRCodeRendererTests.cs(0 hunks)QRCoderTests/TransposeVerificationTests.cs(0 hunks)README.md(0 hunks)
💤 Files with no reviewable changes (28)
- QRCoderTests/QRCodeRendererTests.cs
- QRCoderTests/PayloadGeneratorTests/BitcoinCashAddressTests.cs
- QRCoder/PostscriptQRCode.cs
- QRCoderTests/SvgQRCodeRendererTests.cs
- QRCoderTests/ArtQRCodeRendererTests.cs
- README.md
- QRCoderTests/QRGeneratorTests.cs
- QRCoder/Extensions/StringValueAttribute.cs
- QRCoder/PayloadGenerator.cs
- QRCoder/PayloadGenerator/RussiaPaymentOrder.cs
- QRCoder/QRCodeGenerator/GaloisField.cs
- .github/workflows/wf-verify-formatting.yml
- QRCoderTests/TransposeVerificationTests.cs
- QRCoder/QRCodeGenerator/AlphanumericEncoder.cs
- QRCoder/QRCodeData.cs
- .github/workflows/wf-build-test.yml
- QRCoderApiTests/net60/QRCoder.approved.txt
- QRCoder/ArtQRCode.cs
- QRCoder/QRCodeGenerator/CodewordBlock.cs
- QRCoder/QRCode.cs
- QRCoder/Base64QRCode.cs
- QRCoderTests/Base64QRCodeRendererTests.cs
- QRCoderApiTests/netstandard13/QRCoder.approved.txt
- QRCoderApiTests/netstandard20+netstandard21/QRCoder.approved.txt
- QRCoder/QRCodeGenerator/Polynom.cs
- QRCoder/ASCIIQRCode.cs
- QRCoderTests/PayloadGeneratorTests/LitecoinAddressTests.cs
- QRCoder/PngByteQRCode.cs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-14T18:47:09.346Z
Learnt from: Shane32
PR: Shane32/QRCoder#680
File: QRCoder/Extensions/StringExtensions.cs:33-42
Timestamp: 2025-10-14T18:47:09.346Z
Learning: The `string.StartsWith(char)` method was added in .NET Core 2.0 and .NET Standard 2.1. For conditional compilation in QRCoder, use `!NETCOREAPP2_0_OR_GREATER && !NETSTANDARD2_1_OR_GREATER` to provide the extension method polyfill only for frameworks that lack the built-in method.
Applied to files:
QRCoder/Extensions/StringExtensions.cs
🔇 Additional comments (24)
QRCoderDemo/QRCoderDemo.csproj (1)
4-5: Correctly aligned with PR framework consolidation goals.The demo application has been properly updated to match the new target framework strategy: removing .NET 5.0 support and targeting .NET 6.0 instead. The
UseWindowsFormscondition has been correctly updated to reflect the new target framework.Verify that the main QRCoder library project (QRCoder/QRCoder.csproj) is properly targeting netstandard2.0, netstandard2.1, and net6.0 as stated in the PR objectives, ensuring the demo application dependencies align.
QRCoder/PayloadGenerator/OneTimePassword.cs (1)
177-197: LGTM! Clean refactoring with correct null-forgiving operator usage.The transition from instance methods to static
string.IsNullOrWhiteSpace(...)calls is appropriate, and the null-forgiving operators (!) on lines 188 and 197 are correctly placed after their respective null checks on lines 186 and 195. Sincestring.IsNullOrWhiteSpace(...)returning false guarantees the string is neither null, empty, nor whitespace, the subsequentContains(':')calls are safe.QRCoderTests/PayloadGeneratorTests/RussiaPaymentOrderTests.cs (1)
23-25: Correct encoding provider registration for non-NETFRAMEWORK targets.This conditional registration is necessary and properly placed. Since the PR removes automatic encoding provider registration from the production code, tests that use non-standard encodings (Windows-1251, KOI8-R) must now register
CodePagesEncodingProviderexplicitly for .NET Core/.NET Standard targets.QRCoder/Attributes/StackTraceHiddenAttribute.cs (2)
1-6: LGTM! Correct polyfill pattern.The conditional compilation ensures this polyfill is only included for targets prior to .NET 6.0, where the attribute is built-in. The namespace correctly matches the official
System.Diagnosticslocation.
8-19: LGTM! Polyfill correctly implemented and actively used.The
StackTraceHiddenAttributepolyfill faithfully mirrors the .NET 6.0+ implementation with correctAttributeUsageand sealed class structure. Verification confirms it's being applied to methods in the codebase (e.g., exception-throwing helpers inPolynom.cs), enabling unconditional use across all target frameworks without conditional compilation.QRCoder/PdfByteQRCode.cs (1)
56-60: Mixing StreamWriter and raw stream writes: good flushing discipline—keep it.You flush before capturing offsets, which prevents position skew. No change needed.
Please run a quick smoke test across all TFMs to ensure no newline normalization changes offsets.
Also applies to: 114-116
.github/workflows/wf-build-release-ci.yml (2)
7-7: LGTM: Develop branch added to CI triggers.Adding the develop branch is appropriate for this PR targeting the develop branch.
26-28: LGTM: SDK matrix correctly reduced.Removing .NET SDK 2.1.x and 5.0.x aligns with the PR objectives to eliminate testing on .NET Core 2.1 and .NET 5.0. The remaining SDKs (3.1.x, 6.0.x, 8.0.x) appropriately cover the three target frameworks: .NET Standard 2.0, .NET Standard 2.1, and .NET 6.0.
Directory.Build.props (2)
14-14: LGTM: Version bump to 2.0.0 is appropriate.The major version increment correctly signals the breaking changes in this PR, particularly the removal of support for older target frameworks.
42-42: LGTM: CA1708 suppression added.Adding CA1708 (identifiers should differ by more than case) to the global NoWarn list is reasonable for this codebase.
QRCoder/Attributes/NotNullWhenAttribute.cs (1)
1-1: LGTM: Conditional compilation correctly narrowed.The change from
!NETCOREAPP && !NETSTANDARD2_1toNETSTANDARD2_0is more explicit and accurate. Since the PR targets netstandard2.0, netstandard2.1, and net6.0, only .NET Standard 2.0 lacks the built-inNotNullWhenAttributeand requires this polyfill.QRCoder.Xaml/QRCoder.Xaml.csproj (2)
5-6: LGTM: Target frameworks appropriately consolidated.The reduction from net35/net40/net5.0-windows/net6.0-windows to net462/net6.0-windows aligns with the PR's objective to consolidate framework support. The UseWPF condition is correctly simplified to target only net6.0-windows, as net462 uses framework assembly references instead.
19-23: LGTM: Framework references updated consistently.The ItemGroup condition correctly updated to target only net462, matching the new minimum framework version.
QRCoderConsole/Program.cs (2)
10-12: LGTM: Conditional compilation simplified.Removing the
&& WINDOWScondition while retaining theSupportedOSPlatform("windows")attribute is appropriate. This allows the code to compile on all .NET 6.0 platforms while documenting the Windows requirement for runtime. This aligns with the PR's move toward unconditional System.Drawing usage.
230-232: LGTM: Conditional compilation simplified consistently.Same rationale as the MainClass: removing
&& WINDOWSwhile keepingSupportedOSPlatform("windows")allows broader compilation with documented runtime requirements.QRCoder/Attributes/SupportedOSPlatformAttribute.cs (1)
1-39: LGTM: Well-implemented polyfill attribute.This is a properly implemented polyfill for
SupportedOSPlatformAttributethat enables platform annotation for pre-.NET 6.0 frameworks. The conditional compilation, attribute usage, and empty constructor body follow standard polyfill patterns. TheIDE0060warning suppression is appropriate since the parameter is used only for documentation purposes.QRCoderTests/QRCoderTests.csproj (2)
4-7: LGTM: Test framework matrix correctly updated.The reduction to net462/netcoreapp3.1/net6.0-windows aligns with the PR objectives and appropriately tests all three target TFMs. The conditional properties for UseWindowsForms, UseWPF, and TEST_XAML are correctly scoped to only the frameworks that support these features.
46-48: LGTM: Project reference condition updated consistently.The QRCoder.Xaml project reference is correctly conditioned to only the frameworks that support XAML (net462 and net6.0-windows), consistent with the TEST_XAML define.
QRCoder/QRCodeGenerator/NumericDataSegment.cs (2)
125-129: LGTM: Conditional compilation correctly updated for Span usage.Changing from
HAS_SPANto!NETSTANDARD2_0is more explicit and maintainable. Thestring.AsSpan()extension method is available in .NET Standard 2.1+ and .NET 6.0+ but not in .NET Standard 2.0 (even with System.Memory), so this conditional compilation is necessary and correct.
139-143: LGTM: Consistent Span conditional compilation.Same rationale as above—correctly uses
AsSpanfor .NET Standard 2.1+ and .NET 6.0, with fallback toSubstringfor .NET Standard 2.0.QRCoder/Extensions/StringExtensions.cs (1)
20-24: Span fast-paths and invariant formatting look good.As implemented, AsSpan/TryFormat paths are correctly gated and fall back cleanly on NETSTANDARD2_0.
Also applies to: 38-46, 60-68
QRCoder/QRCodeGenerator.cs (1)
512-516: OK pending BitArrayExtensions fix.Calls to ShiftTowardsBit0/ShiftAwayFromBit0 are correct and simplify alignment. Ensure BitArrayExtensions guard is fixed to avoid netstandard2.1 build breaks.
Run a quick multi-TFM build locally after applying the BitArrayExtensions guard to confirm compilation for netstandard2.1 and net6.0.
Also applies to: 629-633
QRCoderConsole/QRCoderConsole.csproj (1)
4-8: System.Drawing.Common versions are secure; no action required.Verification confirms:
- QRCoderConsole uses version 6.0.0 (latest stable)
- All versions (4.7.3 and 6.0.0) are patched against known CRITICAL RCE vulnerabilities (CVE patches: 4.7.2+ for 4.x; no 5.x vulnerabilities affect 6.0.0)
- No newer stable versions available; 10.x releases are preview/RC only
Code changes are safe to merge.
QRCoder/QRCoder.csproj (1)
27-31: Verify platform compatibility expectations for System.Drawing.Common usage.The conditional versioning strategy (4.7.3 for netstandard targets, 6.0.0 for net6.0) is sound from a compatibility perspective. However, note that System.Drawing.Common 6.0.0 is the latest version that supports non-Windows platforms through a compatibility flag, while versions 7.0+ don't support macOS/Linux anymore. Since .NET 7, non-Windows platforms are not supported, even with runtime configuration switches.
For netstandard2.0 and netstandard2.1 targets using version 4.7.3, this library will only function on Windows at runtime. The PR description mentions moving System.Drawing.Common–dependent components to a separate package in a future PR, which suggests this platform limitation is acknowledged. Ensure this is clearly documented for library consumers.
Confirm that:
- Documentation explicitly states System.Drawing.Common is Windows-only for these targets.
- The future refactoring to separate System.Drawing–dependent components is tracked and prioritized.
- Alternative packages are recommended for cross-platform users.
gfoidl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice cleanup!
| stream.Write(_pdfBinaryComment, 0, _pdfBinaryComment.Length); | ||
| writer.WriteLine(); | ||
|
|
||
| writer.Flush(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is flushing needed? We write to a memory stream, so I don't see real benefit from it.
Same on the other flushes below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's the text writer that requires flushing, not the memory stream. Without flushing, the position may be inaccurate. Now since we are using ASCII encoding, there is (probably) no reason for the encoder to cache characters, but I don't know that it says anywhere that the ASCII encoder doesn't require flushing. So the encoder flushes before reading the position property.
| "%%EOF" // End of file marker | ||
| ); | ||
|
|
||
| writer.Flush(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This flush is fine, as w/o disposing of the StreamWriter not all data written may end up in the (Memory) stream.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we disposed of the stream writer, then we could omit the flush, as flushing is implicit during dispose. However, we are using the shorter using syntax so the flush is required here to ensure all characters have been written.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we are talking about the same. Maybe I was unclear.
This PR:
developbranch (for version 2.0)SpanandArrayPoolcan be used)#ifblocks as possible, adding polyfills where necessary.StringValueAttributeFuture PR:
System.Drawing.Commonto another packageThen, the compilation that targets .NET Standard 2.1 and .NET 6.0 will be dependency-free.
Summary by CodeRabbit
Breaking Changes
StringValueAttributeandCustomExtensionsclasses from public APIChores