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

Throwing FormatException when a required property from Microsoft.Extensions.DependencyModel is empy #60842

Merged
merged 18 commits into from
Nov 2, 2021
Merged

Throwing FormatException when a required property from Microsoft.Extensions.DependencyModel is empy #60842

merged 18 commits into from
Nov 2, 2021

Conversation

filipjelic
Copy link
Contributor

@filipjelic filipjelic commented Oct 25, 2021 •
edited
Loading

Throwing FormatException when a required property is null/empty.
This enables removal of all ! operations addressed in #58139 (comment)

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Oct 25, 2021
@ghost
Copy link

ghost commented Oct 25, 2021

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

Issue Details

Updating property types (strings) to be nullable for:

  • class Target property Name
  • class TargetLibrary property Name
  • class RuntimeTargetEntryStub property Path

This enables removal of all ! operations addressed in #58139 (comment)

Author: filipjelic
Assignees: -
Labels:

area-DependencyModel, community-contribution
Milestone: -

@dnfadmin
Copy link

dnfadmin commented Oct 25, 2021 •
edited
Loading

CLA assistant check
All CLA requirements met.

@filipjelic filipjelic marked this pull request as draft October 25, 2021 23:57
@filipjelic filipjelic marked this pull request as ready for review October 26, 2021 00:19
@filipjelic filipjelic changed the title Setting nullable properties and removing null forgiving operator from Microsoft.Extensions.DependencyModel Setting nullable string properties from Microsoft.Extensions.DependencyModel Oct 26, 2021
@runfoapp runfoapp bot mentioned this pull request Oct 26, 2021
Closed
@filipjelic
Copy link
Contributor Author

@eerhardt Ready for review.

@filipjelic filipjelic mentioned this pull request Oct 27, 2021
Closed
eerhardt
eerhardt reviewed Oct 28, 2021
View reviewed changes
Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

Looks like a good start, @filipjelic. Thanks.

I have left some comments.

src/libraries/Microsoft.Extensions.DependencyModel/src/Dependency.cs Outdated Show resolved Hide resolved
src/libraries/Microsoft.Extensions.DependencyModel/src/ResourceAssembly.cs Outdated Show resolved Hide resolved
src/libraries/Microsoft.Extensions.DependencyModel/src/RuntimeFallbacks.cs Outdated Show resolved Hide resolved
src/libraries/Microsoft.Extensions.DependencyModel/src/RuntimeFile.cs Outdated Show resolved Hide resolved
src/libraries/Microsoft.Extensions.DependencyModel/src/TargetInfo.cs Outdated Show resolved Hide resolved
src/libraries/Microsoft.Extensions.DependencyModel/src/DependencyContextJsonReader.cs Outdated Show resolved Hide resolved
src/libraries/Microsoft.Extensions.DependencyModel/src/DependencyContextJsonReader.cs Outdated Show resolved Hide resolved
src/libraries/Microsoft.Extensions.DependencyModel/src/DependencyContextJsonReader.cs Outdated Show resolved Hide resolved
src/libraries/Microsoft.Extensions.DependencyModel/src/DependencyContextJsonReader.cs Outdated Show resolved Hide resolved
src/libraries/Microsoft.Extensions.DependencyModel/src/DependencyContextJsonReader.cs Show resolved Hide resolved
@runfoapp runfoapp bot mentioned this pull request Oct 28, 2021
Closed
@filipjelic
Copy link
Contributor Author

@eerhardt Code and unit tests have been updated accordingly.
Thanks!

eerhardt
eerhardt reviewed Oct 28, 2021
View reviewed changes
Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

Looking good. Just a few last comments to fix up.

src/libraries/Microsoft.Extensions.DependencyModel/src/DependencyContextJsonReader.cs Outdated Show resolved Hide resolved
src/libraries/Microsoft.Extensions.DependencyModel/src/DependencyContextJsonReader.cs Show resolved Hide resolved
src/libraries/Microsoft.Extensions.DependencyModel/src/DependencyContextJsonReader.cs Outdated Show resolved Hide resolved
eerhardt
eerhardt approved these changes Oct 29, 2021
View reviewed changes
Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the contribution @filipjelic!

@runfoapp runfoapp bot mentioned this pull request Oct 29, 2021
Closed
@filipjelic filipjelic changed the title Setting nullable string properties from Microsoft.Extensions.DependencyModel Throwing FormatException when a required property from Microsoft.Extensions.DependencyModel is empy Oct 30, 2021
@filipjelic filipjelic changed the title Throwing FormatException when a required property from Microsoft.Extensions.DependencyModel is empy Throwing FormatException when a required property from Microsoft.Extensions.DependencyModel is empy Oct 30, 2021
@eerhardt eerhardt merged commit a3f31e9 into dotnet:main Nov 2, 2021
@filipjelic filipjelic deleted the SettingNullableTargetName branch November 2, 2021 19:38
@ghost ghost locked as resolved and limited conversation to collaborators Dec 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@eerhardt eerhardt eerhardt approved these changes

Assignees
No one assigned
Labels
area-DependencyModel community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@filipjelic @dnfadmin @eerhardt