Simplify regex and test on Turkish locale #270
Conversation
|
Great job figuring this out! The docs do actually call out the ignoreCase can cause issues on Turkish locale (see https://msdn.microsoft.com/en-us/library/yd1hzczs.aspx#Invariant ). Using \w or \d would include non-ASCII characters in a .NET RegEx unless ECMAScript mode is enabled, so using the character ranges explicitly is still the best way to go here I think. Other comments on the code. |
| + "(?:-(?<prerelease>[…0-9A-Za-z-]+(\\.[…0-9A-Za-z-]+)*))?" | ||
| + "(?:\\+(?<buildmetadata>[…0-9A-Za-z-]+(\\.[…0-9A-Za-z-]+)*))?$", | ||
| RegexOptions.IgnoreCase | RegexOptions.Singleline); | ||
| + "(?:-(?<prerelease>[…0-9A-Za-z-.]+))?" |
There was a problem hiding this comment.
I get adding the "." to allow multi-part pre-release tags (i.e. "1.0.1-beta.1"), but doesn't this also allow invalid formats such as "1.0.1-..5.."? Also, I though the "-" had to be immediately after the opening "[" or before the closing "]" to be treated as a regular character rather than the range operator.
|
Nice job tracking this down! In addition to Bill's comments... let's go ahead and catch this exception during GetCatalog (and print to the output window) so that people aren't left empty handed if parsing fails (as we do with ArgumentException) |
kant2002
force-pushed
the
issue-253
branch
3 times, most recently
from
3b30052 to
31948ef
Compare
|
Probably was too late to debug this issue, now patch is much more clean. I just have to remove |
| builder.LatestVersion = SemverVersion.Parse(latestVersion); | ||
| try { | ||
| builder.LatestVersion = SemverVersion.Parse(latestVersion); | ||
| } catch (SemverVersionFormatException) { |
There was a problem hiding this comment.
Also print out a message here - we want to know about the issue, but don't want it to block the user.
|
minor comment, but lgtm other than that! |
|
@kant2002 I presume you mistakenly closed this PR since it looks like you restored your branch too, so I went ahead ad re-opened it 😄 |
1. I remove RegexOptions.IgnoreCase since somehow this break the whole recognition on Turkish locale and I don't see any point have it. We already define both uppercase and lower case letters in the regex. Alternative for A-Za-z is to use \w and for 0-9 use \d, so maybe this will lead to more reliable version recognizer. 2. Catch SemverVersionFormatException inside NpmGetCatalogCommand
|
True. I just add reporting of the message in case of invalid package semversion |
Fix #253 Simplify regex and test on Turkish locale
I remove RegexOptions.IgnoreCase since somehow this break the whole recognition on Turkish locale and I don't see any point have it. We already define both uppercase and lower case letters in the regex. Alternative for A-Za-z is to use \w and for 0-9 use \d, so maybe this will lead to more reliable version recognizer. If strict fragment rules needed they should be placed to RegexOptionalFragment, but I was unable to make this work reliable so far.
See #253