-
Notifications
You must be signed in to change notification settings - Fork 84
XML Comments and Feedback from 1-on-1 with Nate #79
Conversation
@muratg for naming. |
@@ -8,15 +8,18 @@ | |||
|
|||
namespace Microsoft.AspNetCore.Rewrite | |||
{ | |||
public static class ModRewriteOptionsExtensions | |||
/// <summary> | |||
/// Apache Mod rewrite extensions on top of the <see cref="RewriteOptions"/> |
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: "Apache mod_rewrite"
{ | ||
public class ChangeEnvironmentPreAction : PreAction | ||
public class ChangeEnvironmentPreAction : UrlAction |
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.
Can you also rename the class "ChangeEnvironmentPreAction" => "ChangeEnvironmentAction"?
⌚ |
🆙 📅 |
{ | ||
public class ChangeEnvironmentPreAction : PreAction | ||
public class ChangeEnvironmentPreAction : UrlAction |
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: sync class name to filename. => "ChangeEnvironmentAction"
LGTM. |
I found a bug for flags, didn't check for the OR flag. Just going to package it here |
|
@@ -81,6 +80,7 @@ public override void ApplyRule(RewriteContext context) | |||
else | |||
{ | |||
request.Path = PathString.FromUriComponent(ForwardSlash + result); | |||
|
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: delete empty line
|
6a9c2e6
to
af2c1ac
Compare
Couple of key things: