Skip to content
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

Implement IUtf8SpanParsable on Guid #105654

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

SirCxyrtyx
Copy link

@SirCxyrtyx SirCxyrtyx commented Jul 29, 2024

Part of #81500

API Proposal: #105653

This implements IUtf8SpanParsable on Guid by making the core of the parsing logic generic on TChar : IUtfChar<TChar>. A small amount of it is duplicated, with a version for char and byte, because doing so prevented a performance regression for char parsing.

In addition, it adds a Utf8 Parse and TryParse without the unused IFormatProvider parameter, to match how ISpanParsable was implemented. I did not add Utf8 overloads for ParseExact, but that can be discussed in the linked API proposal.

@dotnet-issue-labeler dotnet-issue-labeler bot added needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners new-api-needs-documentation labels Jul 29, 2024
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

1 similar comment
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jul 29, 2024
@SirCxyrtyx
Copy link
Author

@dotnet-policy-service agree

//

/// <inheritdoc cref="IUtf8SpanParsable{TSelf}.Parse(ReadOnlySpan{byte}, IFormatProvider?)" />
public static Guid Parse(ReadOnlySpan<byte> utf8Text, IFormatProvider? provider) => Parse(utf8Text);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should these methods with IFormatProvider be explicitly implemented? I see that is the case for e.g. IUtf8SpanFormattable method. C.f. #81500 (comment)

Copy link
Author

Choose a reason for hiding this comment

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

I raised this question in my linked API proposal (#105653), but that was closed as already approved, so I guess this is how it's supposed to be?

Sorted methods in ref assembly.
Removed unneccesary ToSpan().
Unified DecodeByte logic.
Used TrimUtf8() instead of bespoke function.
Update tests to include unicode whitespace characters.
Add IFormatProvider overloads to tests.
@huoyaoyuan huoyaoyuan added area-System.Runtime and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Aug 1, 2024
Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-system-runtime
See info in area-owners.md if you want to be subscribed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Runtime community-contribution Indicates that the PR has been added by a community member new-api-needs-documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants