Conversation
…nce to conditionally use NuGet, and disable CA1850 diagnostic.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdds coverage for UpdateService.ValidateChecksum behavior, introduces analyzer suppressions needed for legacy C# 7.3/.NET Framework constraints, and updates project configuration (including Newtonsoft.Json and CA1850 handling) to align with the test and build environment. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Consider narrowing the
#pragma warning disablescope inUpdateServiceTests.cs(or moving some suppressions into.editorconfig) so that only the specific lines that need each suppression are affected, which will make it easier to spot genuinely new issues in this file.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider narrowing the `#pragma warning disable` scope in `UpdateServiceTests.cs` (or moving some suppressions into `.editorconfig`) so that only the specific lines that need each suppression are affected, which will make it easier to spot genuinely new issues in this file.
## Individual Comments
### Comment 1
<location> `_tests/UpdateServiceTests.cs:72-82` </location>
<code_context>
private static Task TestValidateChecksum_NoChecksum()
{
- // Should return true (allow update with warning)
- if (!UpdateService.ValidateChecksum("Content", null))
+ // Should return true (allow update with warning)
+ if (!UpdateService.ValidateChecksum("Content", null))
throw new Exception("TestValidateChecksum_NoChecksum failed: Null checksum should pass (warn only)");
- if (!UpdateService.ValidateChecksum("Content", ""))
+ if (!UpdateService.ValidateChecksum("Content", ""))
throw new Exception("TestValidateChecksum_NoChecksum failed: Empty checksum should pass (warn only)");
- return Task.CompletedTask;
+ return Task.CompletedTask;
}
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a test case for whitespace-only checksum values in `TestValidateChecksum_NoChecksum`
Since this test already covers null and empty strings, please also verify how `UpdateService.ValidateChecksum` behaves with whitespace-only values (e.g. " ", "\t", or " \t "). Either extend this test with such a case or add a dedicated test documenting the expected behavior for whitespace checksums, so the edge case remains well-defined over time.
```suggestion
private static Task TestValidateChecksum_NoChecksum()
{
// Should return true (allow update with warning)
if (!UpdateService.ValidateChecksum("Content", null))
throw new Exception("TestValidateChecksum_NoChecksum failed: Null checksum should pass (warn only)");
if (!UpdateService.ValidateChecksum("Content", ""))
throw new Exception("TestValidateChecksum_NoChecksum failed: Empty checksum should pass (warn only)");
// Whitespace-only checksums should behave the same as no checksum (warn only, but allow update)
if (!UpdateService.ValidateChecksum("Content", " "))
throw new Exception("TestValidateChecksum_NoChecksum failed: Single-space checksum should pass (warn only)");
if (!UpdateService.ValidateChecksum("Content", "\t"))
throw new Exception("TestValidateChecksum_NoChecksum failed: Tab checksum should pass (warn only)");
if (!UpdateService.ValidateChecksum("Content", " \t "))
throw new Exception("TestValidateChecksum_NoChecksum failed: Mixed whitespace checksum should pass (warn only)");
return Task.CompletedTask;
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…lidation, updated `StreamerBot.csproj` for conditional Newtonsoft.Json usage, and disabled CA1850 diagnostic.
…ation and update `ValidateChecksum` to use `IsNullOrWhiteSpace`.
GitHub's default CodeQL setup is now enabled, which conflicts with the advanced workflow configuration. Removing the advanced workflow file resolves the error: "CodeQL analyses from advanced configurations cannot be processed when the default setup is enabled"
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…nce to conditionally use NuGet, and disable CA1850 diagnostic.
name: Pull Request
about: Propose changes to the bot
title: "[TYPE] Short description of change"
labels: ""
assignees: ""
Description
Type of change
Compatibility Check
new(), norecord, nousing var, etc.).editorconfigHow Has This Been Tested?
_tests/TestRunner.cs!giveaway system testChecklist
Summary by Sourcery
Align tests and project configuration with the .NET Framework 4.8 / C# 7.3 environment and adjust update-related testing and configuration.
Enhancements:
Build:
Tests: