-
Notifications
You must be signed in to change notification settings - Fork 10
Ensure deterministic ordering for Dictionary keys
#22
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
Changes from 4 commits
8cb1f83
8e70638
eb8aaa0
7c9543c
7df821b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -810,6 +810,8 @@ extension TOONEncoder { | |
|
|
||
| private var container: [String: Value] = [:] | ||
| private var keyOrder: [String] = [] // Track insertion order | ||
| // Swift Dictionary encoding uses an internal DictionaryCodingKey; detect it to sort keys deterministically. | ||
| private let isDictionaryCodingKey = String(reflecting: Key.self).contains("DictionaryCodingKey") | ||
|
Comment on lines
+813
to
+814
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Kind of hacky, but it lets us have nice behavior where |
||
|
|
||
| init(encoder: Encoder, codingPath: [CodingKey]) { | ||
| self.encoder = encoder | ||
|
|
@@ -1095,12 +1097,14 @@ extension TOONEncoder { | |
| } | ||
|
|
||
| func finishEncoding() { | ||
| encoder.storage.append(.object(container, keyOrder: keyOrder)) | ||
| let finalKeyOrder = isDictionaryCodingKey ? container.keys.sorted() : keyOrder | ||
| encoder.storage.append(.object(container, keyOrder: finalKeyOrder)) | ||
| } | ||
|
|
||
| deinit { | ||
| // Ensure the container is finished when it goes out of scope | ||
| encoder.storage.append(.object(container, keyOrder: keyOrder)) | ||
| let finalKeyOrder = isDictionaryCodingKey ? container.keys.sorted() : keyOrder | ||
| encoder.storage.append(.object(container, keyOrder: finalKeyOrder)) | ||
|
Comment on lines
+1100
to
+1107
|
||
| } | ||
| } | ||
| } | ||
|
|
||
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.
The detection of Dictionary encoding via String reflection is fragile and relies on internal Swift implementation details. The string "DictionaryCodingKey" is an internal Swift type name that could change in future Swift versions, breaking this functionality silently.
Consider these alternatives:
Given that this is a critical feature for deterministic output, it would be prudent to have explicit test coverage that verifies the detection mechanism is working as expected.