Builder and extensions cleanup #283
Conversation
ghost
added
the
| @@ -20,27 +20,13 @@ public MapWhenMiddleware([NotNull] RequestDelegate next, [NotNull] MapWhenOption | |||
|
|
|||
| public async Task Invoke([NotNull] HttpContext context) | |||
| { | |||
| if (_options.Predicate != null) | |||
| if (_options.Predicate(context)) | |||
There was a problem hiding this comment.
This doesn't need a null check anymore? The setter for this property doesn't seem to guard against null.
There was a problem hiding this comment.
The NotNull guard is here:
https://github.com/aspnet/HttpAbstractions/pull/283/files#diff-93bfbc9b10d86e70f618ce3182ccfbeaL27
It used to check before because there were two different options, sync or async.
There was a problem hiding this comment.
That's a guard on the method, not on the actual options class. Someone can still go to the options and make it null, no?
There was a problem hiding this comment.
With effort. They would have to do app.Use(next => new MapWhenMiddleware(next, options).Invoke); since we don't provide an extension that takes the options directly.
There was a problem hiding this comment.
Hmm, true. Can we add a [NotNull] to the setter anyway? That way there would be no doubt.
|
Where's the implicit conversion from string -> PathString |
|
@davidfowl I didn't find any, just methods like this: https://github.com/aspnet/HttpAbstractions/pull/283/files#diff-d9042ff419b4052d360932870b783853L21 |
|
Ah, I misread that feedback. I thought it was asking to remove any implicit type conversions between string and PathString. I'll update the bug and the PR. |
|
Updated with implicit string converters. |
|
|
1 similar comment
|
|
| @@ -5,12 +5,11 @@ | |||
| using System.Threading.Tasks; | |||
| using Microsoft.AspNet.Http; | |||
| using Microsoft.AspNet.Builder.Extensions; | |||
| using Microsoft.Framework.Internal; | |||
|
|
|||
| namespace Microsoft.AspNet.Builder | |||
There was a problem hiding this comment.
We totally need to change this namespace
#265
Hosting changes to follow.
@davidfowl @muratg