-
Notifications
You must be signed in to change notification settings - Fork 10.4k
Make Kestrels Internals truly internal. #8517
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
Conversation
src/Servers/Kestrel/Core/ref/Microsoft.AspNetCore.Server.Kestrel.Core.netcoreapp3.0.cs
Outdated
Show resolved
Hide resolved
We should also probably have an API review for these changes if we decide to many anything truly public. @sebastienros let's also make sure this doesn't break the platformbenchmark scenario. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code changes look great. I'd get sign off from someone more familiar with the API being made public
I don't know exactly what will be broken, but chances are that this will break it. Here is a list of internal namespace usages: I would suggest to also change PlatformBenchmarks at the same time such that it will work with this PR, or adapt the bits that are used to make them public. Ideally some parts of PlaintextBenchmarks should be in Kestrel and directly reusable in the app. |
Looking at your changes I assume it's actually a good change for the PlatformBenchmarks as some use types will now become really public. Still good if some of the things in PB were in Kestrel directly. |
src/Servers/Kestrel/Core/src/Adapter/ConnectionAdapterContext.cs
Outdated
Show resolved
Hide resolved
src/Servers/Kestrel/Core/src/Adapter/Internal/LoggingConnectionAdapter.cs
Show resolved
Hide resolved
src/Servers/Kestrel/Core/ref/Microsoft.AspNetCore.Server.Kestrel.Core.netcoreapp3.0.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
YESSSS! Overall, I'm super happy about this change.
This is going to make it impossible to write a kestrel transport right? |
The plan is to leave the transport abstractions and a few other things like connection adapters pubternal in this first pass. |
Updated with feedback. I think this is looking muchhhhh better. |
@@ -1,4 +1,4 @@ | |||
// Copyright (c) .NET Foundation. All rights reserved. | |||
// Copyright (c) .NET Foundation. All rights reserved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wish git better tools around normalizing BOMs similar to what it has for line endings gitattributes.
Fixes #8306. Big change here so I'll give you the rundown.
Big Picture:
Nitty Gritty:
For any public API, we probably need to add xml comments appropriately.