-
Notifications
You must be signed in to change notification settings - Fork 308
Conversation
Hi @JunTaoLuo, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution! The agreement was validated by .NET Foundation and real humans are currently evaluating your PR. TTYL, DNFBOT; |
_serverFeatures = ServerFactory.Initialize(_config); | ||
var addresses = _serverFeatures?.Get<IServerAddressesFeature>()?.Addresses; | ||
_server = ServerFactory.CreateServer(_config); | ||
var addresses = _server.Features?.Get<IServerAddressesFeature>()?.Addresses; | ||
if (addresses != null && !addresses.IsReadOnly) |
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.
nit: you could do if (!addresses?.IsReadOnly)
You're off to a good start. Next is moving IHttpContextFactory to Http.Abstractions and HttpContextFactory to Http. Then you need to update TestHost, Kestrel, and Entropy. |
4a9fb84
to
9377483
Compare
|
||
namespace Microsoft.AspNet.Hosting.Server | ||
{ | ||
public interface IServer : IDisposable |
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.
Add some doc comments
e0262a9
to
a65f235
Compare
Based on what Chris said. We should update we host builder and test server to support taking and IServer or ISeverFactory. There's probably an effect on the restartability |
@@ -40,60 +41,65 @@ public void Build_honors_UseStartup_with_string() | |||
[Fact] | |||
public async Task StartupMissing_Fallback() | |||
{ | |||
var application = new RequestDelegate(context => { return Task.FromResult(0); }); |
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.
Why the ceremony here? Shouldn't this just look like it did before?
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.
This will go away once UseServer
takes an IServer
. This hack was added so the RequestDelegate
passed into the IServer
could be accessed for use in AssertResponseContains
.
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.
Alternatively, we can just make this test extend IServer
and IServerFactory
like HostingEngineTests
.
a65f235
to
5543198
Compare
{ | ||
public Func<IFeatureCollection, Task> Application { get; set; } | ||
private Action<RequestDelegate> _setAppDelegate; |
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.
removing this
updated |
/// </summary> | ||
/// <param name="configuration">An instance of <see cref="IConfiguration"/>.</param> | ||
/// <returns>The created server.</returns> | ||
IServer CreateServer(IConfiguration configuration); |
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.
Create or CreateServer?
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.
According to the issue filed, CreateServer.
|
8d21b20
to
aef1a77
Compare
This looks great |
8d2388b
to
3933a19
Compare
#395