-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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 and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix nonsensical RegEx for name restriction #34375
Fix nonsensical RegEx for name restriction #34375
Conversation
"-" character is a range, caused an error. No need for "," to repeat so much, it's not a separator. "\\" - just why?
I think it's to escape the |
Unfortunately it just... Escapes itself by repeating twice. I should've mentioned it, my bad. |
Really? I'm pretty sure the double backslash is needed in C# string literals, and the actual string that regex sees will only contain one backslash before the Edit: It seems like the best practice for regex strings is to use the |
Good to know. Matter of fact, I didn't even notice this. I'll go test it. How would you recommend me to edit the expression to properly escape |
Change it to a |
Added: "@" delimiter for consistency "/" to escape "-" for good and to avoid further problems
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.
Looks good to me, thanks for the fix!
About the PR
Basically I cleaned up the RestrictedNameRegex, mainly by removing unnecessary delimiters (commas) and a backslash, then moved hyphen to the end of the expression for it to not be interpreted as a range of characters.
Why / Balance
Correct regular expression is better than an incorrect one.
Technical details
Unfortunately, for C# and .NET I cannot shorten [A-Za-z0-9] to just \w, as it will include characters from most alphabets and writing systems in unicode.
Clarification for the edited characters:
-
is a range character, has to be taken literally or be at the end, otherwise causes an erroneous expression interpretation.,
to repeat so much, it's not a separator and names should not have commas in them.\\
is just why? It was added alongside the apostrophe symbol in allow ' in character names #28652 for no particular reason. Backslash repeated twice would escape itself and does not seem to affect anything. Replaced by a single backslash\
.@
delimiter for proper interpretation of meta escape characters and for consistency to avoid further problems.Tested and works as intended (no warnings as well).
Media
You can see how the new RegEx filters improper characters in this example.
Requirements
Breaking changes
Uh, IDK, your characters with a name containing a backslash or a comma will no longer contain them.
Changelog
No CL, no fun.