-
-
Couldn't load subscription status.
- Fork 1.2k
Reference System.Drawing.Common for all targets #639
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
📝 WalkthroughWalkthroughAdjusts project target frameworks and System.Drawing package conditions, updates Directory.Build.targets constants, adds explicit solution build steps to CI workflows, and replaces/updates large approved API snapshots for net6.0 and net60-windows test artifacts. No source-code public API signatures were intentionally modified in library sources. Changes
Sequence Diagram(s)sequenceDiagram
participant CI as GitHub Actions runner
participant Build as dotnet build
participant Test as API approval / test steps
Note over CI: Existing workflow job
CI->>+Build: dotnet publish (existing)
CI->>+Build: dotnet build -c Release (NEW)
Build-->>-CI: build result
CI->>+Test: api-approval / tests
Test-->>-CI: test result
rect #e6ffed
note right of CI: New explicit solution build step inserted before tests
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
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: 1
🧹 Nitpick comments (1)
QRCoder/QRCoder.csproj (1)
4-7: Remove the redundant property overrides.These duplicate the
TargetFrameworksand related settings defined a few lines below, so they get overwritten and only add confusion. Please drop the extra block or consolidate into a single property definition.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
QRCoder/QRCoder.csproj(2 hunks)
🧰 Additional context used
🪛 GitHub Actions: Format (Pull Request)
QRCoder/QRCoder.csproj
[error] 42-42: dotnet restore failed. MSB4025: The project file could not be loaded. The 'ItemGroup' start tag on line 33 position 4 does not match the end tag of 'Project'.
|
@gfoidl I'm reconsidering this change (meaning, I'm considering merging this PR). If we look at #489 we see a great number of issues raised due to people not understanding why the various classes are "missing" in .NET 6+. And these are just the people that couldn't find the solution and created an issue; many more people likely had the same problem but solved it themselves. Further, I've needed to help yet another person in the first week I've been maintaining the project. It just seems to me to be simpler for everyone to include the reference to System.Drawing.Common for all target frameworks, letting the .NET Analyzers and/or MS exceptions inform developers if they try to use it on Linux. If someone really wants to omit the System.Drawing.Common reference on Linux, they can stick with QRCoder 1.6.0, or use v2. But if we merge this, it will be binary-compatible with v1.4.x, the lack of which is a common cause of complaint. Comments? |
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.
OK, when it becomes less churn for the community it's good.
This PR is currently opened only for review/discussion.
Summary by CodeRabbit
New Features
Chores