Skip to content

Improved encoding of NSNumber in OpenAPIValueContainer #110

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

Merged
merged 5 commits into from
Jul 19, 2024

Conversation

czechboy0
Copy link
Contributor

Motivation

When getting CoreFoundation/Foundation types, especially numbers, they automatically bridge to Swift types like Bool, Int, etc. That casting is pretty flexible, and allows e.g. casting a number into a boolean, which isn't desired when encoding into JSON, as false and 0 represent very different values.

Previously, we relied on the automatic casting to know how to encode values, however that produced incorrect results in some cases.

Modifications

Add explicit handling of CF/NS types and try to encode using that new method before falling back to testing for native Swift types. This ensures that the original intention of the creator of the CF/NS types doesn't get lost in encoding.

Result

Correct encoding into JSON of types produced in the CF/NS world, like JSONSerialization.

Test Plan

Added unit tests.

@czechboy0 czechboy0 requested a review from simonjbeaumont July 19, 2024 11:20
Copy link
Collaborator

@simonjbeaumont simonjbeaumont left a comment

Choose a reason for hiding this comment

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

Well... this is fun. Nice patch though.

@czechboy0 czechboy0 merged commit 71fcfa7 into apple:main Jul 19, 2024
8 checks passed
@czechboy0 czechboy0 deleted the hd-nsnumber-encoding branch July 19, 2024 11:32
@czechboy0 czechboy0 added the 🔨 semver/patch No public API change. label Jul 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔨 semver/patch No public API change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants