feat : Add `StringSyntax` for regex parameters by ShreyasJejurkar · Pull Request #40589 · dotnet/aspnetcore (original) (raw)

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation22 Commits7 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

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! :)

namespace Microsoft.AspNetCore.Rewrite.ApacheModRewrite;

internal class RuleRegexParser
{
public static ParsedModRewriteInput ParseRuleRegex(string regex)
public static ParsedModRewriteInput ParseRuleRegex([StringSyntax(StringSyntaxAttribute.Regex)] string regex)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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! :)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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))
{

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Includes: MVC, Actions and Controllers, Localization, CORS, most templates

label

Mar 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me on the routing side, although it's not very useful since a user doesn't call this constructor directly.

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.