feat : Add StringSyntax for regex parameters
#40589
Conversation
ghost
added
the
community-contribution
ShreyasJejurkar
requested review from
a team,
Tratcher,
BrennanConroy and
javiercn
as code owners
| namespace Microsoft.AspNetCore.Rewrite.ApacheModRewrite; | ||
|
|
||
| internal class RuleRegexParser | ||
| { | ||
| public static ParsedModRewriteInput ParseRuleRegex(string regex) | ||
| public static ParsedModRewriteInput ParseRuleRegex([StringSyntax(StringSyntaxAttribute.Regex)] string regex) |
There was a problem hiding this comment.
This is an internal class. Does it matter whether this is attributed? It would only affect a developer working on this library and passing in a literal string. No existing call sites take literal strings. I'd revert this and all other similar cases.
There was a problem hiding this comment.
Yes, I have reverted this. But shouldn't we consider the internal developer's productivity working on the project? Whether the type is public or not, the string still holds a special meaning (in this case which is regex) and should be annotated so that developer would get a sense of what needs to pass here!
Am totally ok with this approach of excluding internal types, but I think having attributes on internal types is helpful as well! :)
There was a problem hiding this comment.
It's pretty obvious from the parameter name what this is (and the attribute doesn't show up in intellisense whereas the parameter name does) , and there's zero functional benefit to the attribute if the argument isn't a literal, which it never is here. From my perspective, attributing such things is just noise that actually harms the dev experience.
| @@ -14,7 +15,7 @@ internal class RedirectRule : IRule | |||
| public Regex InitialMatch { get; } | |||
| public string Replacement { get; } | |||
| public int StatusCode { get; } | |||
| public RedirectRule(string regex, string replacement, int statusCode) | |||
| public RedirectRule([StringSyntax(StringSyntaxAttribute.Regex)] string regex, string replacement, int statusCode) | |||
| { | |||
| if (string.IsNullOrEmpty(regex)) | |||
| { | |||
There was a problem hiding this comment.
I can't comment on it, but as an aside, the | RegexOptions.CultureInvariant below is a nop. CultureInvariant only means something when IgnoreCase is also specified.
mkArtakMSFT
added
the
area-mvc
|
Thanks for your PR, @ShreyasJejurkar. |
If the parameter is never used with literals at the call site, I don't think we should bother using the attribute. At that point it only clutters up the code without actually adding value. |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
No functional changes.
Visual Studio understands this attribute and colorizes the parameter as string holds special meaning.
Tagging @stephentoub here in case if anything to improve about attribute usage! :)