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.

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

Enable Nullability in multiple Design classes #8390

Merged
merged 11 commits into from
Jan 6, 2023

Conversation

elachlan
Copy link
Contributor

@elachlan elachlan commented Dec 15, 2022

Enable Nullability in multiple Design classes

Related: #8342

Microsoft Reviewers: Open in CodeFlow

@elachlan elachlan requested a review from a team as a code owner December 15, 2022 09:09
@ghost ghost assigned elachlan Dec 15, 2022
@elachlan elachlan changed the title Enable Nullability in ImageListImageEditor Enable Nullability in multiple Design classes Dec 15, 2022
@elachlan elachlan force-pushed the ImageListImageEditor-Nullability branch from b9ce87b to 136f84e Compare December 15, 2022 10:09
@lonitra lonitra added the waiting-author-feedback The team requires more information from the author label Dec 19, 2022
@ghost ghost removed the waiting-author-feedback The team requires more information from the author label Dec 19, 2022
@elachlan elachlan requested a review from lonitra December 19, 2022 23:05
@elachlan
Copy link
Contributor Author

@lonitra I think the test failures are unrelated?

@@ -48,12 +46,12 @@ protected static string CreateExtensionsString(string[] extensions, string sep)
protected static string CreateFilterEntry(IconEditor editor)
{
string description = editor.GetFileDialogDescription();
string extensions = CreateExtensionsString(editor.GetExtensions(), ",");
string extensionsWithSemicolons = CreateExtensionsString(editor.GetExtensions(), ";");
string? extensions = CreateExtensionsString(editor.GetExtensions(), ",");
Copy link
Member

Choose a reason for hiding this comment

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

I think there is some inconsistency here. The first parameter in CreateExtensionsString() is nullable, but it looks like every time CreateExtensionsString() is called we are passing in the return value of GetExtensions() which is currently nonnullable. Could you verify whether or not GetExtensions() ever returns a null value and adjust accordingly?

Copy link
Contributor Author

@elachlan elachlan Dec 22, 2022

Choose a reason for hiding this comment

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

GetExtensions always returns a non-null value. CreateExtensionsString is protected so it is externally accessible. So a null value is possible.

I've removed the nullability on CreateExtensionsString parameter extensions. I think its best to let the user know that we expect a value. Since if you pass null, we return null.

Copy link
Member

Choose a reason for hiding this comment

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

There are still checks on line 28. If this were made publicly accessible, what is the benefit to modify the behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I might have been wrong here. In a stricter sense, if we throw a ArguementNullException or ArguementException, then it would make more sense to annotate that we don't want nulls. But because we handle it in code, I think we should have the nullability.

I think the idea is that if a null input is going to cause an error or invalid state, then we annotate it not null. This isn't the case here.

Copy link
Member

Choose a reason for hiding this comment

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

Correct, while we like to make it explicit with respect to nullability, in this case, we are handling null values and it's been exposed publicly. We should mark it nullable for compatibility even when we internally never pass null. Our documentation is also doesn't enforce this.

@ghost ghost added waiting-author-feedback The team requires more information from the author and removed waiting-author-feedback The team requires more information from the author labels Dec 22, 2022
@dreddy-work dreddy-work added the waiting-author-feedback The team requires more information from the author label Jan 5, 2023
@ghost ghost removed the waiting-author-feedback The team requires more information from the author label Jan 5, 2023
dreddy-work
dreddy-work previously approved these changes Jan 5, 2023
@dreddy-work dreddy-work added the ready-to-merge PRs that are ready to merge but worth notifying the internal team. label Jan 5, 2023
@dreddy-work dreddy-work requested a review from lonitra January 5, 2023 22:58
@elachlan elachlan force-pushed the ImageListImageEditor-Nullability branch from 9f3a608 to 665c32c Compare January 5, 2023 23:29
@elachlan
Copy link
Contributor Author

elachlan commented Jan 5, 2023

I rebased due to a clash. Apologies for that. The last commit is really the only change.

@dreddy-work dreddy-work merged commit cf48c6d into dotnet:main Jan 6, 2023
@ghost ghost added this to the 8.0 Preview1 milestone Jan 6, 2023
@elachlan elachlan deleted the ImageListImageEditor-Nullability branch January 6, 2023 19:39
@ghost ghost locked as resolved and limited conversation to collaborators Feb 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
ready-to-merge PRs that are ready to merge but worth notifying the internal team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants