-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Fix #if conditional to match rest of application #613
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
📝 WalkthroughWalkthroughReplaced conditional compilation symbol NET5_0_OR_GREATER with NETCOREAPP2_1_OR_GREATER || NETSTANDARD2_1 in QRCodeGenerator.PlainTextToBinaryNumeric, switching numeric parsing from Substring to AsSpan for fixed-width digit groups (3-digit groups, 2-digit remainder, 1-digit remainder). No public API changes. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ 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). (1)
🔇 Additional comments (3)
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 |
| { | ||
| // Parse the next three characters as a decimal integer. | ||
| #if NET5_0_OR_GREATER | ||
| #if NETCOREAPP2_1_OR_GREATER || NETSTANDARD2_1 |
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 NETCOREAPP2_1_OR_GREATER || NETSTANDARD2_1 | |
| #if NETCOREAPP || NETSTANDARD2_1 |
I thinks this could be shortened for better legibility.
Older .NET Core targets than 2.1 aren't present. And .NET (5+) is covered by that compiler constant too.
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.
Well, as written:
- it matches the rest of the codebase
- it matches the conditions for which Span is supported
So I'd prefer to leave it as-is here for those reasons. Perhaps it would be better to have a compile time constant defined for span support - similar to SYSTEM_DRAWING we have SPAN - then in the csproj we define it for the applicable target frameworks. Then we change all uses to match.
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.
Your suggestion with a #define HAS_SPAN (or similar) sound reasonable as follow-up work.
Summary by CodeRabbit