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

[GB18030] Fix SubstringByTextElements - avoid ArgumentOutOfRangeException #10080

Merged
merged 6 commits into from Apr 30, 2024

Conversation

ilonatommy
Copy link
Member

@ilonatommy ilonatommy commented Apr 29, 2024

Follow up for #10063, see the discussion.

Context

The original PR was fixing GB18030 issue but for project names with less text elements than 8 it would be throwing an exception. What's more, SubstringByTextElements used previously is giving different results for .net framework and .net core, see doc.

Changes Made

Fix: do not change project name if it's shorter than 8 chars. Replace non-ASCII chars with underscore _, cut substring to the desired length.

Testing

Automatic test case added.

@ilonatommy ilonatommy self-assigned this Apr 29, 2024
Copy link
Member

@JanKrivanek JanKrivanek left a comment

Choose a reason for hiding this comment

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

Thanks!

src/Build/Evaluation/IntrinsicFunctions.cs Outdated Show resolved Hide resolved
Co-authored-by: Jan Krivanek <krivanek.j@hotmail.com>
Copy link
Member

@JanKrivanek JanKrivanek left a comment

Choose a reason for hiding this comment

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

Looks good!

Let's make sure we cannot end up with invalid filname characters (as a result of breaking the multichar sequence)

src/Build/Evaluation/IntrinsicFunctions.cs Outdated Show resolved Hide resolved
Copy link
Member

@JanKrivanek JanKrivanek left a comment

Choose a reason for hiding this comment

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

Thank you!
Looks solid

@JanKrivanek JanKrivanek merged commit ab42402 into dotnet:main Apr 30, 2024
10 checks passed
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.

None yet

4 participants