Skip to content

Conversation

@maxkoshevoi
Copy link
Contributor

@maxkoshevoi maxkoshevoi commented Dec 2, 2021

Fixes

  • Add nullable annotations (C# 8 nullable support)
  • Initialize (some) collection properties
  • Style fixes related to new C# capabilities (e.g. default(T) -> default)
  • Fix httpErrorAsException argument not used in SendGridClient.cs

Checklist

  • I acknowledge that all my contributions will be made under the project's license
  • I have made a material change to the repo (functionality, testing, spelling, grammar)
  • I have read the Contribution Guidelines and my PR follows them
  • I have titled the PR appropriately
  • I have updated my branch with the main branch
  • I have added tests that prove my fix is effective or that my feature works
  • I have added the necessary documentation about the functionality in the appropriate .md file
  • I have added inline documentation to the code I modified

If you have questions, please file a support ticket, or create a GitHub Issue in this repository.

Copy link

@Felix200521 Felix200521 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

src/SendGrid/Helpers/Mail/Model/Personalization.cs

@maxkoshevoi
Copy link
Contributor Author

maxkoshevoi commented Jan 5, 2022

Ok, I'm not sure how to build this thing using latest .net sdk (6.0). Could someone please update the PR to do so?
#1110 might be enough though.

Update: Never mind, reverted C#9 changes

@childish-sambino childish-sambino changed the title Nullable support feat: Nullable support Mar 28, 2022
@childish-sambino childish-sambino changed the title feat: Nullable support feat: add Nullable support Mar 28, 2022
@maxkoshevoi maxkoshevoi changed the title feat: add Nullable support feat: Add Nullable support Apr 4, 2022
@childish-sambino
Copy link
Contributor

Code Review: Nullable Reference Types Implementation

This PR introduces C# 8.0 nullable reference types to the SendGrid C# library. Here's my analysis:

✅ Positive Aspects

  1. Proper Configuration: The .csproj files correctly enable nullable reference types with <Nullable>enable</Nullable>, <WarningsAsErrors>nullable</WarningsAsErrors>, and <LangVersion>8</LangVersion>

  2. Polyfill Package: Added the Nullable package (v1.3.0) for backward compatibility with older target frameworks

  3. Consistent Annotations: The changes systematically add ? annotations to parameters and properties that can legitimately be null

  4. Smart Initialization: Key collections like Tos in Personalization.cs are initialized to empty lists (= new List<EmailAddress>()) which addresses the concern about required fields

  5. Null-Forgiving Operator Usage: Appropriate use of ! operator after null checks (e.g., in MailHelper.cs after !string.IsNullOrEmpty() checks)

  6. Code Modernization: Nice updates like default(CancellationToken)default and formatting improvements

⚠️ Concerns & Suggestions

  1. Stale PR: This PR has been inactive since April 2022. It needs rebasing against current main branch and resolving conflicts.

  2. DisallowNull on Nullable Properties: The use of [DisallowNull] on nullable properties like EmailAddress? From is unusual. Consider whether these should be truly non-nullable with initialization or nullable without the attribute.

  3. Breaking Changes: Nullable annotations may cause compile-time warnings for consumers. Consider:

    • Documenting as potentially breaking
    • Version bump if using semantic versioning
    • Migration guidance
  4. Test Coverage: The checklist mentions "Tests pending." Need unit tests validating nullable behavior before merge.

  5. CI Status: Should verify CI passes before merge.

🔍 Specific Code Issue

BaseClient.cs line ~204:

request.Content.Headers.ContentType!.CharSet = null;

This null-forgiving operator is risky. Consider adding an explicit null check instead.

📋 Next Steps

  1. Rebase against current main branch
  2. Resolve conflicts
  3. Add nullable-specific tests
  4. Update documentation
  5. Ensure CI passes

Conclusion

Solid foundational work for modernizing the codebase with good understanding of nullable reference types. However, given staleness and pending items, significant updates needed before merge.


Automated review via Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants