feat : Add StringSyntax for regex parameters#40589
Conversation
| 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.
| 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.
|
Thanks for your PR, @ShreyasJejurkar. |
javiercn
left a comment
There was a problem hiding this comment.
Looks good to me on the routing side, although it's not very useful since a user doesn't call this constructor directly.
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! :)