Adding / Updating comments for all public APIs - #53
Conversation
Co-authored-by: SteveWinward <2002602+SteveWinward@users.noreply.github.com>
Co-authored-by: SteveWinward <2002602+SteveWinward@users.noreply.github.com>
Co-authored-by: SteveWinward <2002602+SteveWinward@users.noreply.github.com>
Co-authored-by: SteveWinward <2002602+SteveWinward@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
The updated documentation introduces mismatches with current behavior and the SheetRange.Equals implementations have confirmed null/type-safety bugs that should be fixed before release.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR primarily improves XML documentation across several public APIs in the GoogleSheetsWrapper library and bumps the NuGet package version for the next patch release.
Changes:
- Updated/expanded XML doc comments for multiple public APIs to better describe behavior and parameters/returns.
- Bumped
GoogleSheetsWrapperpackage version from2.0.26to2.0.27. - Minor doc wording cleanup (e.g., correcting “mulitlpe” → “multiple”).
File summaries
| File | Description |
|---|---|
| src/GoogleSheetsWrapper/Utils/PhoneNumberParsing.cs | Updated method XML docs for phone-number normalization/conversion utilities. |
| src/GoogleSheetsWrapper/SheetRange.cs | Updated XML docs for notation properties, constructors, and equality/hash-code methods. |
| src/GoogleSheetsWrapper/SheetHelper.cs | Updated XML docs for initialization and various CRUD/helper operations. |
| src/GoogleSheetsWrapper/SheetFieldAttributeUtils.cs | Updated XML docs for reflection-based attribute utilities. |
| src/GoogleSheetsWrapper/SheetExporter.cs | Updated XML docs for CSV/Excel export helpers. |
| src/GoogleSheetsWrapper/GoogleSheetsWrapper.csproj | Bumped package version to 2.0.27. |
Review details
Suppressed comments (3)
src/GoogleSheetsWrapper/Utils/PhoneNumberParsing.cs:35
- This doc describes converting an already-normalized phone number and returning the number "without its country code", but the implementation also strips "+1" and non-digits itself (and not necessarily only leading). The summary/returns should describe what the method actually does so callers know they can pass formatted input.
/// <summary>
/// Converts a normalized United States phone number to a numeric value.
/// </summary>
/// <param name="number">The phone number to convert.</param>
/// <returns>The numeric phone number without its country code.</returns>
src/GoogleSheetsWrapper/SheetRange.cs:289
Equals(object obj)performs an unchecked cast toSheetRange, which can throwInvalidCastExceptionfor non-SheetRangeinputs (andNullReferenceExceptionindirectly). Override equality should returnfalsefor other types and nulls.
public override bool Equals(object obj)
{
return Equals((SheetRange)obj);
}
src/GoogleSheetsWrapper/SheetRange.cs:271
Equals(SheetRange other)dereferencesotherwithout a null check, sorange.Equals((SheetRange)null)will throw aNullReferenceExceptioninstead of returningfalse(as expected byIEquatable<T>and many callers). Add a null guard before comparing fields.
This issue also appears on line 286 of the same file.
public bool Equals(SheetRange other)
{
return
A1Notation == other.A1Notation &&
CanSupportA1Notation == other.CanSupportA1Notation &&
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: SteveWinward <2002602+SteveWinward@users.noreply.github.com>
Updates the library NuGet package version for the next patch release.
GoogleSheetsWrapperversion to2.0.27.