Repository navigation
Fix NRT errors and enable TreatWarningsAsErrors - #46
Conversation
- Enable TreatWarningsAsErrors in build - Fix nullable reference annotations in Aliyun storage implementation
There was a problem hiding this comment.
Pull request overview
Updates the Aliyun storage provider and build configuration to support stricter compilation (warnings as errors) by addressing nullable-reference-type (NRT) warnings and aligning dependency versions.
Changes:
- Bumped
Foundatio/Foundatio.TestHarnesspackage references to13.0.0-beta3.35. - Adjusted Aliyun storage/provider option and connection-string builder nullability annotations.
- Minor internal refactors related to nullability initialization.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Directory.Build.props | Updates Foundatio.TestHarness package version. |
| src/Foundatio.Aliyun/Foundatio.Aliyun.csproj | Updates Foundatio package version. |
| src/Foundatio.Aliyun/Storage/AliyunFileStorageOptions.cs | Changes ConnectionString nullability/initialization. |
| src/Foundatio.Aliyun/AliyunConnectionStringBuilder.cs | Changes connection-string parts (Endpoint, AccessKey, SecretKey) nullability + ctor signature. |
| src/Foundatio.Aliyun/Storage/AliyunFileStorageConnectionStringBuilder.cs | Changes _bucket init and ctor signature nullability. |
| src/Foundatio.Aliyun/Storage/AliyunFileStorage.cs | Updates SearchCriteria.Prefix initialization for NRT. |
Comments suppressed due to low confidence (1)
src/Foundatio.Aliyun/AliyunConnectionStringBuilder.cs:20
- Changing the constructor parameter from
string?tostringis a source-breaking nullability change for any consumers/derivers that may currently passnull(even though it throws at runtime). If you want to preserve compatibility while still validating, consider keeping thestring?signature (or adding an overload) and continuing to throw for null/empty input.
protected AliyunConnectionStringBuilder() { }
protected AliyunConnectionStringBuilder(string connectionString)
{
if (String.IsNullOrEmpty(connectionString))
throw new ArgumentNullException(nameof(connectionString));
Parse(connectionString);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Address PR review comments: properties annotated as non-nullable string were initialized with null!, making the nullability contract inaccurate. Initialize with string.Empty instead so the non-nullable annotation is truthful at runtime.
…ring.Empty Tests assert null for unset properties. Using string.Empty changed the semantic meaning of not-set from null to empty, breaking 3 tests. Properly nullable properties preserve original behavior.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/Foundatio.Aliyun/AliyunConnectionStringBuilder.cs:20
String.IsNullOrEmptyallows whitespace-only connection strings (e.g., " "), whichParsewill effectively treat as having no options (since there are no=pairs). With the constructor now non-nullable, consider switching this guard toString.IsNullOrWhiteSpaceso obviously invalid inputs are rejected immediately and consistently.
protected AliyunConnectionStringBuilder(string connectionString)
{
if (String.IsNullOrEmpty(connectionString))
throw new ArgumentNullException(nameof(connectionString));
Parse(connectionString);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Change connection string validation from ThrowIfNullOrEmpty to ThrowIfNullOrWhiteSpace - Modernize options null check to use ArgumentNullException.ThrowIfNull
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
Test plan