-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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 encoding missed overridden properties #113167
Fix encoding missed overridden properties #113167
Conversation
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.
PR Overview
This PR fixes missing support for several encoding properties (WindowsCodePage, IsBrowserDisplay, IsBrowserSave, IsMailNewsDisplay, and IsMailNewsSave) that previously threw NotSupportedException.
- Adds new tests and MemberData to verify the proper mapping of code pages to Windows code pages and flags.
- Implements overrides in EncodingNLS and updates EncodingTable and EncodingTable.Data to provide the correct MIME flags and family code pages using caching.
Reviewed Changes
File | Description |
---|---|
src/libraries/System.Text.Encoding.CodePages/tests/EncodingCodePages.cs | Added tests for WindowsCodePage and flag properties and introduced new MemberData with the code page mapping. |
src/libraries/System.Text.Encoding.CodePages/src/System/Text/EncodingNLS.cs | Implemented new overrides for encoding properties using the cached code page item lookup. |
src/libraries/System.Text.Encoding.CodePages/src/System/Text/EncodingTable.Data.cs | Defined new MIME flag constants and populated MappedFlags with the appropriate values. |
src/libraries/System.Text.Encoding.CodePages/src/System/Text/EncodingTable.cs | Added a cache and implemented GetCodePageItem to return family code page and flags. |
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
src/libraries/System.Text.Encoding.CodePages/tests/EncodingCodePages.cs:782
- Rather than silently returning when the encoding is null, consider asserting that encoding is not null so that unexpected missing encodings are flagged in the test results.
if (encoding is null) // Not supported encoded like utf cases
src/libraries/System.Text.Encoding.CodePages/src/System.Text.Encoding.CodePages.csproj
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Encoding.CodePages/src/System/Text/EncodingNLS.cs
Show resolved
Hide resolved
src/libraries/System.Text.Encoding.CodePages/src/System/Text/EncodingNLS.cs
Show resolved
Hide resolved
src/libraries/System.Text.Encoding.CodePages/src/System/Text/EncodingNLS.cs
Outdated
Show resolved
Hide resolved
Do you need to re-run the ref source update for this assembly? https://github.com/dotnet/runtime/blob/main/docs/coding-guidelines/updating-ref-source.md Edit: Nevermind, internal class. |
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.
LGTM. Feel free to consider or ignore my suggestion for the returned tuple values.
…ncodingNLS.cs Co-authored-by: Carlos Sánchez López <1175054+carlossanlop@users.noreply.github.com>
….com/tarekgh/runtime into FixEncodingMissedOverridenProperties
@@ -91,6 +91,7 @@ System.Text.CodePagesEncodingProvider</PackageDescription> | |||
<ItemGroup Condition="'$(TargetFrameworkIdentifier)' != '.NETCoreApp'"> | |||
<PackageReference Include="System.Memory" Version="$(SystemMemoryVersion)" /> | |||
<PackageReference Include="System.Runtime.CompilerServices.Unsafe" Version="$(SystemRuntimeCompilerServicesUnsafeVersion)" /> | |||
<PackageReference Include="System.ValueTuple" Version="$(SystemValueTupleVersion)" /> |
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.
@MichaelSimons @ViktorHofer this package reference is causing the error in CI about prebuilts.
I tried adding the package to source-build-reference-packages but I get this error:
~/repos/source-build-reference-packages $ ./generate.sh --package System.ValueTuple,4.5.0
...
Build succeeded in 4.6s
Restore complete (1.9s)
PackageSourceGeneratorTask succeeded (2.9s) → artifacts/bin/PackageSourceGeneratorTask/Debug/net10.0/PackageSourceGeneratorTask.dll
PackageSourceGenerator succeeded (0.4s) → Package source generation skipped for System.ValueTuple (v4.5.0) as it doesn't contain any compile items.
What options do we have for such a case where no compile items are found in a published package, but adding such package as reference causes prebuilt errors?
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.
Replied offline already. The System.ValueTuple package is a netfx support package that doesn't need to be referenced on NET Standard.
Make sure that the package ref is conditioned to TargetFrameworkIdentifier == NETFramework.
/ba-g the failures are infrastructure and looks temporary. |
Fixes #113059
When we introduced the codepage encodings library, we missed supporting the encoding properties
WindowsCodePage
,IsBrowserDisplay
,IsBrowserSave
,IsMailNewsDisplay
, andIsMailNewsSave
. The current behavior is these properties will throwNotSupportedException
.Some users hit this issue when migrating from .NET Framework to .NET.