Skip to content

Conversation

@davidwengier
Copy link
Member

All of our formatting passes took an IClientConnection but only Html actually uses it. Cleaning this up gives us a more accurate picture for cohosting.

@davidwengier davidwengier requested a review from a team as a code owner May 13, 2024 00:50
_documentMappingService = documentMappingService;
_clientConnection = clientConnection;
}
private readonly IRazorDocumentMappingService _documentMappingService = documentMappingService;
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you're removing the ArgumentNullException, could IRazorDocumentMappingService ever be null?

Copy link
Member Author

Choose a reason for hiding this comment

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

This class is directly constructed in code, so nullable reference types protects us from ever passing in null. If it was created by DI, then both of the DI technologies we use would throw errors during composition/container creation and never get this far anyway.

Copy link
Member

Choose a reason for hiding this comment

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

The fact that both MEF and MS.Ext.DependencyInjection already protect against null is a good one to remember. We have lots of services and endpoints that validate against null arguments that are wholly unnecessary. As more and more code is nullable enabled (most of tooling is and most of the compiler is not), removing unnecessary ArgumentNullExceptions gets a thumbs up from me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants