Skip to content

Enable nullability on System.Xml.XPath - #41060

Merged
jozkee merged 2 commits into
dotnet:masterfrom
jozkee:nullability_5
Aug 20, 2020
Merged

Enable nullability on System.Xml.XPath#41060
jozkee merged 2 commits into
dotnet:masterfrom
jozkee:nullability_5

Conversation

@jozkee

@jozkeejozkee commented Aug 19, 2020•
edited
Loading

Copy link
Copy Markdown
Member

Contributes to #2339
No annotations are needed for the APIs exposed in this assembly.
Add missing annotations to XPathException.

@jozkeejozkee added this to the 5.0.0 milestone Aug 19, 2020
@jozkee
jozkee requested review from buyaa-n and krwqAugust 19, 2020 21:50
@jozkeejozkee self-assigned this Aug 19, 2020
@ghost

ghost commented Aug 19, 2020

Copy link
Copy Markdown

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

@stephentoub

stephentoub commented Aug 19, 2020•
edited
Loading

Copy link
Copy Markdown
Member

No annotations are needed for the APIs exposed in this assembly.

That doesn't sound right. For example, I see there's an XpathException type, and its message and innerException ctor arguments should be nullable.

@buyaa-n

buyaa-n commented Aug 19, 2020

Copy link
Copy Markdown
Contributor

Constructor overloads accept nullable
https://source.dot.net/#System.Private.CoreLib/Exception.cs,32

@jozkee

jozkee commented Aug 19, 2020

Copy link
Copy Markdown
MemberAuthor

@stephentoub@buyaa-n you are right, I missed the message parameter in the src annotations as well. Will add them here.

jeffhandley
jeffhandley approved these changes Aug 19, 2020
View reviewed changes

@jeffhandleyjeffhandley left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. @buyaa-n can you take a look too?

Never mind -- I see you already did; I needed to refresh.

buyaa-n
buyaa-n approved these changes Aug 19, 2020
View reviewed changes

@buyaa-nbuyaa-n left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, thanks!

@jeffhandley

jeffhandley commented Aug 20, 2020

Copy link
Copy Markdown
Member

@jozkee Is there anything left on this one, or is it ready for merge? Did you want a review from @krwq first?

@jozkee

jozkee commented Aug 20, 2020

Copy link
Copy Markdown
MemberAuthor

@jeffhandley I think is fine, will merge now...

@jozkee
jozkee merged commit 6891633 into dotnet:masterAug 20, 2020
@jozkee
jozkee deleted the nullability_5 branch August 20, 2020 05:41
@krwq

krwq commented Aug 20, 2020

Copy link
Copy Markdown
Member

LGTM thanks!

@carlossanlopcarlossanlop mentioned this pull request Aug 25, 2020
Merged
14 tasks
@ghostghost locked as resolved and limited conversation to collaborators Dec 7, 2020
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jeffhandleyjeffhandleyjeffhandley approved these changes
@krwqkrwqAwaiting requested review from krwq
+1 more reviewer
@buyaa-nbuyaa-nbuyaa-n approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@jozkeejozkee

Labels

area-System.Xml

Projects

None yet

Milestone

5.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@jozkee@stephentoub@buyaa-n@jeffhandley@krwq