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

fix(66229): Fix invalid processing of empty Dictionary node. #86485

Merged
merged 1 commit into from
May 20, 2023
Merged

fix(66229): Fix invalid processing of empty Dictionary node. #86485

merged 1 commit into from
May 20, 2023

Conversation

Maximys
Copy link
Contributor

@Maximys Maximys commented May 19, 2023

Reopened #86394
Fixes of #66229

For rerun failed tests by CI.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label May 19, 2023
@ghost
Copy link

ghost commented May 19, 2023

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

Issue Details

Reopened #86394
Fixes of #66229

For rerun failed tests by CI.

Author: Maximys
Assignees: -
Labels:

area-Extensions-Configuration, community-contribution
Milestone: -

@Maximys Maximys force-pushed the bugfix/66229-processing-empty-dictionary-items branch from f01d0b8 to c41c884 Compare May 19, 2023 06:00
This was referenced May 19, 2023
Open
Closed
@Maximys Maximys force-pushed the bugfix/66229-processing-empty-dictionary-items branch from c41c884 to bd7d032 Compare May 19, 2023 12:35
@Maximys
Copy link
Contributor Author

Maximys commented May 19, 2023

@tarekgh , could you review my current change again? If I correctly understand logs of currently failed tests, then it's linked with unavailable iOS device.

@Maximys
Copy link
Contributor Author

Maximys commented May 19, 2023

/azp run runtime

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 86485 in repo dotnet/runtime

tarekgh
tarekgh reviewed May 19, 2023
View reviewed changes
src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs Outdated
else
{
BindProperties(bindingPoint.Value, config, options);
bindingPoint.SetValue(CreateInstance(type, config, options));
Copy link
Member

Choose a reason for hiding this comment

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

SetValue

This should be TrySetValue to avoid the case when having the property is readonly..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed by amend: a8944dd#diff-2e6f75e1d4f577e1c8c1b6a32d158ce7812a38ad8fcd3b4d762157b92c03e890R420

@tarekgh
Copy link
Member

tarekgh commented May 19, 2023

@Maximys the failure is not related to your change, and it is known failure https://github.com/dotnet/runtime/pull/86485/checks?check_run_id=13615931751. I guess you are going to fix the code according to my comment #86485 (comment) which will trigger the CI one more time anyway.

@layomia the fix here looks good to me. Let me know if you are ok with it too.

@tarekgh tarekgh added this to the 8.0.0 milestone May 19, 2023
@build-analysis build-analysis bot mentioned this pull request May 20, 2023
Closed
3 tasks
@Maximys Maximys force-pushed the bugfix/66229-processing-empty-dictionary-items branch from bd7d032 to a8944dd Compare May 20, 2023 09:55
@danmoseley
Copy link
Member

Did some tabs creep in? We do 4 space indent

@Maximys
Copy link
Contributor Author

Maximys commented May 20, 2023

Did some tabs creep in? We do 4 space indent

No, changes of spacing linked with this line. This condition was extracted from this line

@tarekgh tarekgh marked this pull request as ready for review May 20, 2023 17:50
@tarekgh tarekgh merged commit e7e9dbd into dotnet:main May 20, 2023
@tarekgh tarekgh mentioned this pull request May 20, 2023
Closed
@Maximys Maximys deleted the bugfix/66229-processing-empty-dictionary-items branch June 6, 2023 04:33
@ghost ghost locked as resolved and limited conversation to collaborators Jul 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@tarekgh tarekgh tarekgh approved these changes

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

Successfully merging this pull request may close these issues.
3 participants
@Maximys @tarekgh @danmoseley