-
Notifications
You must be signed in to change notification settings - Fork 90
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
Revert to treating PackageDownload as Development Dependency #1320
Conversation
While we do know that some PackageDownloads are used at runtime and have active alerts against them - the user action to fix those isn't centered around updating the dependency, it's instead about updating the SDK. �As such, we feel that treating them as non-development dependencies now may result in user confusion and lot lead to the right result.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1320 +/- ##
=====================================
Coverage 89.7% 89.7%
=====================================
Files 387 387
Lines 30271 30272 +1
Branches 1871 1871
=====================================
+ Hits 27158 27159 +1
Misses 2713 2713
Partials 400 400 ☔ View full report in Codecov by Sentry. |
src/Microsoft.ComponentDetection.Detectors/nuget/NuGetPackageReferenceFrameworkAwareDetector.cs
Show resolved
Hide resolved
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.
Adding some tests to avoid regressions will help here.
Also fix up some other misuses of AllSatisfy.
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.
Copilot reviewed 2 out of 2 changed files in this pull request and generated no suggestions.
Comments skipped due to low confidence (4)
test/Microsoft.ComponentDetection.Detectors.Tests/nuget/NuGetPackageReferenceFrameworkAwareDetectorTests.cs:279
- The assertion should ensure the key exists before checking its value. Consider using
c => componentRecorder.GetEffectiveDevDependencyValue(c.Component.Id).HasValue.Should().BeTrue()
to ensure the key exists.
detectedComponents.Should().AllSatisfy(c => componentRecorder.GetEffectiveDevDependencyValue(c.Component.Id).Should().BeTrue(), "All should be development dependencies");
test/Microsoft.ComponentDetection.Detectors.Tests/nuget/NuGetPackageReferenceFrameworkAwareDetectorTests.cs:292
- The assertion should ensure the key exists before checking its value. Consider using
c => componentRecorder.GetEffectiveDevDependencyValue(c.Component.Id).HasValue.Should().BeTrue()
to ensure the key exists.
detectedComponents.Should().AllSatisfy(c => componentRecorder.GetEffectiveDevDependencyValue(c.Component.Id).Should().BeTrue(), "All should be development dependencies");
test/Microsoft.ComponentDetection.Detectors.Tests/nuget/NuGetPackageReferenceFrameworkAwareDetectorTests.cs:332
- The assertion should ensure the key exists before checking its value. Consider using
c => componentRecorder.GetEffectiveDevDependencyValue(c.Component.Id).HasValue.Should().BeTrue()
to ensure the key exists.
dependencies.Should().AllSatisfy(c => componentRecorder.GetEffectiveDevDependencyValue(c.Component.Id).Should().BeTrue(), "All PackageDownloads should be development dependencies");
src/Microsoft.ComponentDetection.Detectors/nuget/NuGetPackageReferenceFrameworkAwareDetector.cs:194
- The word 'develeopment' is misspelled. It should be 'development'.
// Conservatively assume that PackageDownloads are not develeopment dependencies even though usage will not effect any runtime behavior.
👋 Hi! It looks like you modified some files in the
If none of the above scenarios apply, feel free to ignore this comment 🙂 |
While we do know that some PackageDownloads are used at runtime and have active alerts against them - the user action to fix those isn't centered around updating the dependency, it's instead about updating the SDK. �As such, we feel that treating them as non-development dependencies now may result in user confusion and lot lead to the right result.