Skip to content

Conversation

@BrennanConroy
Copy link
Member

@BrennanConroy BrennanConroy commented Aug 21, 2023

Also throw if the ServiceProvider doesn't support keyed services.

Recommend reviewing with whitespace hidden: https://github.com/dotnet/aspnetcore/pull/50248/files?diff=split&w=1

@BrennanConroy BrennanConroy added the area-signalr Includes: SignalR clients and servers label Aug 21, 2023
@BrennanConroy BrennanConroy added this to the 8.0-rc2 milestone Aug 21, 2023
@adityamandaleeka
Copy link
Member

cc @benjaminpetit

{
if (marked)
{
throw new InvalidOperationException(
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should unify this with the exceptions throw in MVC/minimal APIs?

RE:

throw new NotSupportedException(

Copy link
Member Author

Choose a reason for hiding this comment

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

That one doesn't mention which method is the culprit, maybe because it can be an unnamed delegate?

I'd like to keep the name for SignalR so it's very clear where the issue is. Maybe just add it at the beginning:
"{methodExecutor.MethodInfo.FullName}: The {nameof(FromKeyedServicesAttribute)} is not supported on parameters that are also annotated with {nameof(IFromServiceMetadata)}."

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated the exception message

@adityamandaleeka
Copy link
Member

LGTM, is it ready for merge?

@BrennanConroy
Copy link
Member Author

Yeah

@adityamandaleeka adityamandaleeka merged commit 51367f6 into release/8.0 Sep 5, 2023
@adityamandaleeka adityamandaleeka deleted the brecon/di2 branch September 5, 2023 03:11
@ghost ghost modified the milestone: 8.0-rc2 Sep 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-signalr Includes: SignalR clients and servers

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants