-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
Clarify XML comments for Utf8Parser.TryParse to reflect case insensitivity #103861
base: main
Are you sure you want to change the base?
Conversation
/// <param name="bytesConsumed">On a successful parse, receives the length in bytes of the substring that was parsed </param> | ||
/// <param name="standardFormat">Expected format of the Utf8 string</param> | ||
/// <param name="bytesConsumed">On a successful parse, receives the length in bytes of the substring that was parsed</param> | ||
/// <param name="standardFormat">Expected format of the Utf8 string. Supported formats are G and l.</param> |
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.
A clarification on formats has been added because the current implementation checks the format without using it afterwards.
Lines 31 to 32 in 8e92aef
if (!(standardFormat == default(char) || standardFormat == 'G' || standardFormat == 'l')) | |
ThrowHelper.ThrowFormatException_BadFormatSpecifier(); |
Should this check be removed?
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.
This is intentional so the logic isn't blocked from supporting future format specifiers
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.
Ok. I won't change this validation.
Thank you, @joegoldman2. I'd like @tannergooding or @GrabYourPitchforks to review particularly your question that you left about whether the check should be removed. It might be another week or two though. |
src/libraries/System.Private.CoreLib/src/System/Buffers/Text/Utf8Parser/Utf8Parser.Boolean.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Buffers/Text/Utf8Parser/Utf8Parser.Boolean.cs
Outdated
Show resolved
Hide resolved
/// <param name="bytesConsumed">On a successful parse, receives the length in bytes of the substring that was parsed </param> | ||
/// <param name="standardFormat">Expected format of the Utf8 string</param> | ||
/// <param name="bytesConsumed">On a successful parse, receives the length in bytes of the substring that was parsed</param> | ||
/// <param name="standardFormat">Expected format of the Utf8 string. Supported formats are G and l. The default value is also accepted.</param> |
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.
What about:
/// <param name="standardFormat">Expected format of the Utf8 string. Supported formats are G and l. The default value is also accepted.</param> | |
/// <param name="standardFormat">Expected format of the Utf8 string. Supported formats are <c>'G'</c>, <c>'l'</c>, and <c>default</c>.</param> |
Fixes #100659.
I will also update the doc in https://github.com/dotnet/dotnet-api-docs if this PR is accepted.