Skip to content

Add DictionaryKeyPolicy support for EnumConverter [#47765] - #54429

Merged
layomia merged 5 commits into
dotnet:mainfrom
SkiFoD:skifod/issue-47765
Aug 3, 2021
Merged

Add DictionaryKeyPolicy support for EnumConverter [#47765]#54429
layomia merged 5 commits into
dotnet:mainfrom
SkiFoD:skifod/issue-47765

Conversation

@SkiFoD

@SkiFoDSkiFoD commented Jun 18, 2021•
edited
Loading

Copy link
Copy Markdown
Contributor

Sorry for the delay. I added the DictionaryKeyPolicy support for the EnumConverter + made two tests to cover the behavior.

@ghost

ghost commented Jun 18, 2021

Copy link
Copy Markdown

Tagging subscribers to this area: @eiriktsarpalis, @layomia
See info in area-owners.md if you want to be subscribed.

Issue Details
Author:SkiFoD
Assignees:-
Labels:

area-System.Text.Json

Milestone:-

@SkiFoD
SkiFoD requested a review from layomiaJuly 13, 2021 10:20
@terrajobstterrajobst added the community-contribution Indicates that the PR has been added by a community member label Jul 19, 2021
layomia
layomia reviewed Jul 23, 2021
View reviewed changes
Comment thread...raries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/EnumConverter.cs Outdated
Comment thread...raries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/EnumConverter.cs Outdated
Comment thread...raries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/EnumConverter.cs Outdated
Comment thread...raries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/EnumConverter.cs Outdated
@layomialayomia mentioned this pull request Jul 23, 2021
Closed
@SkiFoD
SkiFoD requested a review from layomiaJuly 27, 2021 15:02
layomia
layomia reviewed Jul 27, 2021
View reviewed changes

@layomialayomia left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks really close, just a few minor comments & the merge conflict needs to be resolved.

Comment thread...raries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/EnumConverter.cs Outdated
Comment thread...raries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/EnumConverter.cs Outdated
Comment thread...raries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/EnumConverter.cs Outdated
Comment thread...raries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/EnumConverter.cs Outdated
Comment thread...raries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/EnumConverter.cs Outdated
Comment thread...System.Text.Json.Tests/Serialization/CollectionTests/CollectionTests.Dictionary.KeyPolicy.cs Outdated
@SkiFoD
SkiFoD requested a review from layomiaJuly 27, 2021 21:26
@karelzkarelz mentioned this pull request Jul 28, 2021
Closed
layomia
layomia approved these changes Aug 3, 2021
View reviewed changes

@layomialayomia left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, thanks.

@layomialayomia added the bug label Aug 3, 2021
@layomia
layomia merged commit 814fb1b into dotnet:mainAug 3, 2021
@SkiFoD

SkiFoD commented Aug 3, 2021

Copy link
Copy Markdown
ContributorAuthor

Thank you for the guidance through the issue. It's cool to be able to participate :).

@danmoseley

danmoseley commented Aug 3, 2021•
edited
Loading

Copy link
Copy Markdown
Contributor

Thanks for contributing @SkiFoD ! There are many other JSON issues and if there of one you find interesting we would welcome another contribution from you!

thaystg added a commit to thaystg/runtime that referenced this pull request Aug 3, 2021
@thaystg
Merge remote-tracking branch 'origin/main' into thays_fix_56249
c0e7fb2
* origin/main: (64 commits)
[wasm][debugger] Create test Inherited Properties (dotnet#56754)
Mark new test as incompatible with GC Mark4781_1GcStressIncompatible (dotnet#56739)
Ensure MetadataEnumResult is sufficiently updated by MetaDataImport::Enum (dotnet#56756)
[mono] Remove gdb xdebug and binary writer support, it hasn't worked in a while. (dotnet#56759)
Update windows-requirements.md (dotnet#56476)
Update doc and generic parameter name for JsonValue.GetValue (dotnet#56639)
[wasm][debugger] Inspect static class (dotnet#56740)
Fix stack overflow handling issue in GC stress (dotnet#56733)
Use ReflectionOnly as serialization mode in case dynamic code runtime feature is not supported (dotnet#56604)
Move Windows Compat pack to NuGet pack task (dotnet#56686)
Fix build error when building some packages (dotnet#56767)
Simplify JIT shutdown logic in crossgen2 (dotnet#56687)
Fix race in crossdac publishing with PGO (dotnet#56762)
Add DictionaryKeyPolicy support for EnumConverter [dotnet#47765] (dotnet#54429)
Use ComWrappers in some Marshal unit-tests and update platform metadata (dotnet#56595)
Set `DisableImplicitNamespaceImports_Dotnet=true` to workaround sdk issue (dotnet#56744)
Make sure ServerGCHeapDetails is up to date (dotnet#56056)
[libraries] Reenable System.Diagnostics.DiagnosticSorce.Switches.Tests on mobile (dotnet#56737)
Disable failing arm64 win10 Graphics.FromHdc tests (dotnet#56732)
Match xplat event source conditions (dotnet#56435)
...
thaystg added a commit to thaystg/runtime that referenced this pull request Aug 4, 2021
@thaystg
Merge remote-tracking branch 'origin/main' into thays_implement_debug…
33652d5
…ger_proxy_attribute
* origin/main: (340 commits)
add RID for Debian 11 (dotnet#56789)
[wasm] [debugger] Skip thread static field (dotnet#56749)
Fix timeouts in coreroot_determinism test in GC stress mode (dotnet#56770)
Use File.OpenHandle in Socket.SendFile directly (dotnet#56777)
accept empty realm for digest auth (dotnet#56369) (dotnet#56455)
[wasm][debugger] Create test Inherited Properties (dotnet#56754)
Mark new test as incompatible with GC Mark4781_1GcStressIncompatible (dotnet#56739)
Ensure MetadataEnumResult is sufficiently updated by MetaDataImport::Enum (dotnet#56756)
[mono] Remove gdb xdebug and binary writer support, it hasn't worked in a while. (dotnet#56759)
Update windows-requirements.md (dotnet#56476)
Update doc and generic parameter name for JsonValue.GetValue (dotnet#56639)
[wasm][debugger] Inspect static class (dotnet#56740)
Fix stack overflow handling issue in GC stress (dotnet#56733)
Use ReflectionOnly as serialization mode in case dynamic code runtime feature is not supported (dotnet#56604)
Move Windows Compat pack to NuGet pack task (dotnet#56686)
Fix build error when building some packages (dotnet#56767)
Simplify JIT shutdown logic in crossgen2 (dotnet#56687)
Fix race in crossdac publishing with PGO (dotnet#56762)
Add DictionaryKeyPolicy support for EnumConverter [dotnet#47765] (dotnet#54429)
Use ComWrappers in some Marshal unit-tests and update platform metadata (dotnet#56595)
...
@ghostghost locked as resolved and limited conversation to collaborators Sep 2, 2021
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@eiriktsarpaliseiriktsarpalisAwaiting requested review from eiriktsarpalis
@jozkeejozkeeAwaiting requested review from jozkee
@stevehartersteveharterAwaiting requested review from steveharter
1 more reviewer
@layomialayomialayomia approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-System.Text.Jsonbugcommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@SkiFoD@danmoseley@layomia@terrajobst