Skip to content
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

InvariantCulture flag for Regex with IgnoreCase flag #2901

Merged
merged 11 commits into from
Mar 18, 2020

Conversation

dominoFire
Copy link
Contributor

Bug

Fixes: NuGet/Home#8246
Regression: Yes

  • Last working version: dotnet.exe 2.2.x
  • How are we preventing it in future: Add more integration tests

Fix

Adds Culture Invariant comparison for path matching for files

Testing/Validation

Tests Added: Yes
Validation: Manual validation on dotnet (Linux Version) in WSL.

@dominoFire dominoFire marked this pull request as ready for review June 21, 2019 18:31
@danmoseley
Copy link
Contributor

danmoseley commented Jun 21, 2019

@dominoFire I see 8 other instances in NuGet.Client of RegexOptions.IgnoreCase that does not also have RegexOptions.CultureInvariant. Several them seem to be applying to file system paths. Do any of hte others need the same change?

cc @tarekgh @ericstj

Copy link
Member

@nkolev92 nkolev92 left a comment

Choose a reason for hiding this comment

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

Maybe we should make the changes to all the regex's dealing with apths like @danmosemsft and @ericstj are suggesting.

@zivkan
Copy link
Member

zivkan commented Jun 28, 2019

+1 to using RegexOptions.CultureInvariant in more places. I don't think that NuGet ever needs a culture-specific regex, so I suggest using RegexOptions.CultureInvariant in every Regex (but check regex expressions, in case I'm wrong).

@danmoseley
Copy link
Contributor

@dominoFire are you planning to complete this PR? It has broken our build in the past so we'd be happy to see the fix merged. thanks!

@nkolev92
Copy link
Member

nkolev92 commented Mar 6, 2020

@dominoFire 🔔

Please either address feedback or close.

@dominoFire dominoFire changed the title Invariant culture comparison for regex path matching InvariantCulture flag for Regex with IgnoreCase flag Mar 9, 2020
@dominoFire dominoFire force-pushed the dev-dominofire-pack-uppercase-fix branch from 634058a to f84ad59 Compare March 10, 2020 22:39
Copy link
Contributor

@aortiz-msft aortiz-msft left a comment

Choose a reason for hiding this comment

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

🕐

@dominoFire dominoFire force-pushed the dev-dominofire-pack-uppercase-fix branch from 6209e7b to 212e96c Compare March 18, 2020 03:00
@dominoFire dominoFire requested review from nkolev92, kartheekp-ms and aortiz-msft and removed request for JarahSi March 18, 2020 16:32
@dominoFire dominoFire force-pushed the dev-dominofire-pack-uppercase-fix branch from 212e96c to 7d9c58c Compare March 18, 2020 19:40
@aortiz-msft
Copy link
Contributor

extra empty line


Refers to: test/NuGet.Core.Tests/NuGet.Protocol.Tests/ServiceTypesTests.cs:25 in 8c4f7c7. [](commit_id = 8c4f7c7, deletion_comment = False)

Copy link
Contributor

@aortiz-msft aortiz-msft left a comment

Choose a reason for hiding this comment

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

:shipit:

@dominoFire dominoFire merged commit 4ec73f8 into dev Mar 18, 2020
@dominoFire dominoFire deleted the dev-dominofire-pack-uppercase-fix branch March 18, 2020 22:09
@danmoseley
Copy link
Contributor

thanks @dominoFire

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pack, and several other code paths, fails dependant on locale. Use RegexOptions.CultureInvariant
6 participants