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

Update JsonNew for IReference+cleaner optionals, and better Mappers #6890

Merged
1 commit merged into from
Jul 13, 2020

Conversation

DHowett
Copy link
Member

@DHowett DHowett commented Jul 12, 2020

This commit updates JsonUtilsNew to support winrt
Windows::Foundation::IReference<T> as an option type, and cleans up the
optional support code by removing the optional overload on
GetValue(...). Instead of using an overload with a partial
specialization, we're using a constexpr if with a type trait to
determine option-type-ness.

In addition, Carlos reported an issue with deriving from FlagMapper
(itself templated) and referring to the base type's members without
fully qualifying them. To make derivation easier, EnumMapper and
FlagMapper now provide BaseEnumMapper and BaseFlagMapper type
aliases.

I've taken the opportunity to add a winrt::hstring conversion
trait.

Lastly, in casual use, I found out that I'd written the til::color
converter wrong: it supports color strings of length 7 (#rrggbb) and
length 4 (#rgb). I mistyped (and failed to test) support for 4-length
color strings by pretending they were only 3 characters long.

References

Merged JsonUtils changes from #6004 and #6590.

PR Checklist

  • Unblocks aforementioned PRs
  • cla
  • Tests added/passed
  • Documentation N/A
  • Schema N/A
  • Kid tested, mother approved.

This commit updates JsonUtilsNew to support winrt
Windows::Foundation::IReference<T> as an option type, and cleans up the
optional support code by removing the optional overload on
GetValue(...). Instead of using an overload with a partial
specialization, we're using a constexpr if with a type trait to
determine option-type-ness.

In addition, Carlos reported an issue with deriving from FlagMapper
(itself templated) and referring to the base type's members without
fully qualifying them. To make derivation easier, `EnumMapper` and
`FlagMapper` now provide `BaseEnumMapper` and `BaseFlagMapper` type
aliases.

Lastly, in casual use, I found out that I'd written the til::color
convreter wrong: it supports color strings of length 7 (#rrggbb) and
length 4 (#rgb). I mistyped (and failed to test) support for 4-length
color strings by pretending they were only 3 characters long.
@DHowett DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jul 12, 2020
@ghost
Copy link

ghost commented Jul 12, 2020

Hello @DHowett!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

Do note that I've been instructed to only help merge pull requests of this repository that have been opened for at least 8 hours. No worries though, I will be back when the time is right! 😉

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

Copy link
Member

@carlos-zamora carlos-zamora 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!

@ghost ghost merged commit 89c4eba into master Jul 13, 2020
@ghost ghost deleted the dev/duhowett/ju-new-updates branch July 13, 2020 16:47
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AutoMerge Marked for automatic merge by the bot when requirements are met
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants