Skip to content

Enable nullability in BindToObject#9554

Merged
dreddy-work merged 6 commits intodotnet:mainfrom
gpetrou:BindToObject
Jul 26, 2023
Merged

Enable nullability in BindToObject#9554
dreddy-work merged 6 commits intodotnet:mainfrom
gpetrou:BindToObject

Conversation

@gpetrou
Copy link
Copy Markdown
Contributor

@gpetrou gpetrou commented Jul 24, 2023

Proposed changes

  • Enable nullability in BindToObject.
Microsoft Reviewers: Open in CodeFlow

@gpetrou gpetrou requested a review from a team as a code owner July 24, 2023 15:54
@ghost ghost assigned gpetrou Jul 24, 2023
@ghost ghost added the area-NRT label Jul 24, 2023
/// in the current row
/// </summary>
private string GetErrorText(object value)
private string GetErrorText(object? value)
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.

can text local nullable and avoid duplicate assignment?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sorry, what do you mean?

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.

local on line 109 can be declared as string? text ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Actually, I see that text can never be null. Maybe we should remove ?? string.Empty; instead from line 130?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@dreddy-work I added a separate commit with the proposed change.

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.

final change could alter functionality. Though Interface says default is empty string, I see implementations of this interface returning null either for errorInfo[FieldInfo.Name] or errorInfo.Error. So, i would go with declaration as string? text and return text??striong.Empty;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I kept the removal of text and added some null checks instead.

@dreddy-work dreddy-work added the waiting-author-feedback The team requires more information from the author label Jul 24, 2023
@ghost ghost removed the waiting-author-feedback The team requires more information from the author label Jul 24, 2023
return true;
}

if (!(_owner.DataSource is ISupportInitializeNotification ds) || ds.IsInitialized)
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.

Suggested change
if (!(_owner.DataSource is ISupportInitializeNotification ds) || ds.IsInitialized)
if (_owner.DataSource is not ISupportInitializeNotification ds || ds.IsInitialized)

CheckBinding();
}

internal void SetBindingManagerBase(BindingManagerBase lManager)
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.

Are you sure lManager can't be null? I was under the impression the BindingManagerBase property whose setter calls this method can be null.

}
}

internal void SetValue(object value)
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.

Same here, I thought the argument could be null on the call site.

@dreddy-work dreddy-work added the waiting-author-feedback The team requires more information from the author label Jul 25, 2023
@ghost ghost removed the waiting-author-feedback The team requires more information from the author label Jul 25, 2023
@gpetrou gpetrou force-pushed the BindToObject branch 2 times, most recently from b0533c9 to 64e883a Compare July 25, 2023 18:07
@dreddy-work dreddy-work added the waiting-author-feedback The team requires more information from the author label Jul 25, 2023
@ghost ghost removed the waiting-author-feedback The team requires more information from the author label Jul 25, 2023
@dreddy-work dreddy-work merged commit 32d5b29 into dotnet:main Jul 26, 2023
@ghost ghost added this to the 8.0 RC1 milestone Jul 26, 2023
@gpetrou gpetrou deleted the BindToObject branch July 26, 2023 05:09
@ghost ghost locked as resolved and limited conversation to collaborators Aug 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants