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

Fix StringSegment.Buffer can be null #57858

Merged
merged 11 commits into from
Aug 24, 2021
Merged

Conversation

maxkoshevoi
Copy link
Contributor

@maxkoshevoi maxkoshevoi commented Aug 20, 2021

Addresses: #57395 (comment)

  • Made Buffer nullable
  • Equals(text) returns true if both Buffer and text are null

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Aug 20, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added area-Extensions-Primitives new-api-needs-documentation and removed community-contribution Indicates that the PR has been added by a community member labels Aug 20, 2021
@dotnet-issue-labeler
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, to 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.

@ghost
Copy link

ghost commented Aug 20, 2021

Tagging subscribers to this area: @eerhardt, @maryamariyan
See info in area-owners.md if you want to be subscribed.

Issue Details

Addresses: #57395 (comment)

  • Made Buffer nullable
  • Return original implementation of Equals (throws on null)
Author: maxkoshevoi
Assignees: -
Labels:

new-api-needs-documentation, area-Extensions-Primitives

Milestone: -

@drieseng
Copy link
Contributor

drieseng commented Aug 22, 2021

Perhaps also add a test where null is passed to .ctor(String), and assert that:

  • Buffer returns null
  • Offset return zero
  • Length returns zero
  • HasValue returns false
  • Value returns null
  • ...

@maxkoshevoi
Copy link
Contributor Author

maxkoshevoi commented Aug 22, 2021

There is one already: https://github.com/dotnet/runtime/blob/main/src/libraries/Microsoft.Extensions.Primitives/tests/StringSegmentTest.cs#L164
Nullable is not enabled for tests, so didn't notice it when doing #57395

Update: Added more asserts in it

@drieseng
Copy link
Contributor

@maxkoshevoi Missed that one, thx.

@JamesNK
Copy link
Member

JamesNK commented Aug 23, 2021

FYI this PR is blocking the runtime -> aspnetcore merge: dotnet/aspnetcore#35547

If it is in a good state (@eerhardt or @stephentoub could you review the latest changes?) then it would be great to get it merged and unblock updates.

@eerhardt eerhardt merged commit 4a7a49b into dotnet:main Aug 24, 2021
@stephentoub
Copy link
Member

Thanks for fixing it.

@maxkoshevoi maxkoshevoi deleted the mk/primitives2 branch August 24, 2021 02:34
@ghost ghost locked as resolved and limited conversation to collaborators Sep 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants