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.

Sign up for GitHub

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

Jump to bottom

Use NullabilityInfoContext in DataAnnotationsMetadataProvider #39219

Merged
merged 2 commits into from
Jan 4, 2022
Merged

Use NullabilityInfoContext in DataAnnotationsMetadataProvider #39219

merged 2 commits into from
Jan 4, 2022

Conversation

captainsafia
Copy link
Member

Closes #38774.

@ghost ghost added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Dec 28, 2021
@captainsafia captainsafia force-pushed the safia/nullable-context-provider branch from 15ed8a0 to 2063212 Compare December 29, 2021 17:19
pranavkm
pranavkm approved these changes Dec 30, 2021
View reviewed changes
Copy link
Contributor

@pranavkm pranavkm left a comment

Choose a reason for hiding this comment

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

Looks great

src/Mvc/Mvc.DataAnnotations/src/DataAnnotationsMetadataProvider.cs Outdated
Comment on lines 27 to 28
private const string NullableAttributeFullTypeName = "System.Runtime.CompilerServices.NullableAttribute";
private const string NullableFlagsFieldName = "NullableFlags";
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this also be removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's still used in this method (ref) which is used here where we don't have access to the PropertyInfo and only the attributes on it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would say it might be ok to remove that code now. We'll have enough time in this release to know if we break someone (super unlikely)

@captainsafia captainsafia marked this pull request as ready for review January 3, 2022 20:57
@captainsafia captainsafia requested a review from javiercn as a code owner January 3, 2022 20:57
pranavkm
pranavkm approved these changes Jan 3, 2022
View reviewed changes
Copy link
Contributor

@pranavkm pranavkm left a comment

Choose a reason for hiding this comment

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

:shipit:

@captainsafia captainsafia enabled auto-merge (squash) January 4, 2022 00:02
@captainsafia captainsafia merged commit b2c2cb0 into dotnet:main Jan 4, 2022
@ghost ghost added this to the 7.0-preview1 milestone Jan 4, 2022
@brunolins16 brunolins16 mentioned this pull request Jan 21, 2022
Closed
6 tasks
@brunolins16 brunolins16 mentioned this pull request May 10, 2022
Closed
1 task
@bkoelman bkoelman mentioned this pull request May 14, 2022
Closed
1 task
@serpent5 serpent5 mentioned this pull request Apr 20, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pranavkm pranavkm pranavkm approved these changes

@javiercn javiercn Awaiting requested review from javiercn

Assignees
No one assigned
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates
Projects
None yet
Milestone
7.0-preview1
Development

Successfully merging this pull request may close these issues.

Consider updating DataAnnotationsMetadataProvider to use NullabilityInfoContext
2 participants
@captainsafia @pranavkm