-
Notifications
You must be signed in to change notification settings - Fork 104
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 Decoding of Empty String #145
Merged
MaxDesiatov
merged 23 commits into
CoreOffice:master
from
bwetherfield:fix-empty-string-empty-elt
Nov 13, 2019
Merged
Changes from 21 commits
Commits
Show all changes
23 commits
Select commit
Hold shift + click to select a range
f2ce406
Treat corner case of empty string value intrinsic
jsbean 3adaa40
Remove xcodeproj junk
jsbean e544f76
Add some logging to assess where we're at
jsbean 5ea4fa7
Add cases for empty string as null element decoding
bwetherfield f9fc2f4
Swiftformat
bwetherfield 2e40e6a
Merge pull request #47 from bwetherfield/wetherfield/empty-element-em…
jsbean 3ccbfb3
Transform precondition to where clause in switch statement
jsbean 800a26a
Remove print statements
jsbean c4dabba
Remove nested syntax test
bwetherfield 64f016c
Merge branch 'master' into fix-empty-string-empty-elt
bwetherfield 29ee5d9
Merge branch 'master' into fix-empty-string-empty-elt
MaxDesiatov 4bac491
Fix forcecast
bwetherfield 882eca2
Merge branch 'master' into fix-empty-string-empty-elt
bwetherfield e14594b
Fix formatting
bwetherfield a966be6
Merge branch 'fix-empty-string-empty-elt' of https://github.com/bweth…
bwetherfield 8b99000
Update LinuxMain
bwetherfield 0222d24
Remove warnings
bwetherfield 023dc0d
Merge branch 'master' into fix-empty-string-empty-elt
bwetherfield 27cbb5d
Add fix for new empty string issue
bwetherfield 085a12b
Add back missing missingTest - remove "value" special casing from imp…
bwetherfield 8691214
Fix formatting
bwetherfield f4209dd
Recover missing test!
bwetherfield e7347b3
Update linuxmain
bwetherfield File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,71 @@ | ||
// | ||
// EmptyElementEmptyStringTests.swift | ||
// XMLCoderTests | ||
// | ||
// Created by James Bean on 9/29/19. | ||
// | ||
|
||
import XCTest | ||
import XMLCoder | ||
|
||
class EmptyElementEmptyStringTests: XCTestCase { | ||
struct Parent: Equatable, Codable { | ||
let thing: Thing | ||
} | ||
|
||
struct Thing: Equatable, Codable { | ||
let attribute: String? | ||
let value: String | ||
|
||
enum CodingKeys: String, CodingKey { | ||
case attribute | ||
case value = "" | ||
} | ||
} | ||
|
||
func testEmptyElementEmptyStringDecoding() throws { | ||
let xml = """ | ||
<thing></thing> | ||
""" | ||
let expected = Thing(attribute: nil, value: "") | ||
let result = try XMLDecoder().decode(Thing.self, from: xml.data(using: .utf8)!) | ||
XCTAssertEqual(expected, result) | ||
} | ||
|
||
func testEmptyElementEmptyStringWithAttributeDecoding() throws { | ||
let xml = """ | ||
<thing attribute="x"></thing> | ||
""" | ||
let expected = Thing(attribute: "x", value: "") | ||
let result = try XMLDecoder().decode(Thing.self, from: xml.data(using: .utf8)!) | ||
XCTAssertEqual(expected, result) | ||
} | ||
|
||
func testArrayOfEmptyElementStringDecoding() throws { | ||
let xml = """ | ||
<container> | ||
<thing></thing> | ||
<thing attribute="x"></thing> | ||
<thing></thing> | ||
</container> | ||
""" | ||
let expected = [ | ||
Thing(attribute: nil, value: ""), | ||
Thing(attribute: "x", value: ""), | ||
Thing(attribute: nil, value: ""), | ||
] | ||
let result = try XMLDecoder().decode([Thing].self, from: xml.data(using: .utf8)!) | ||
XCTAssertEqual(expected, result) | ||
} | ||
|
||
func testNestedEmptyElementEmptyStringDecoding() throws { | ||
let xml = """ | ||
<parent> | ||
<thing/> | ||
</parent> | ||
""" | ||
let expected = Parent(thing: Thing(attribute: nil, value: "")) | ||
let result = try XMLDecoder().decode(Parent.self, from: xml.data(using: .utf8)!) | ||
XCTAssertEqual(expected, result) | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Does this test pass? If so, I'd prefer to keep it in the test suite
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.
I've played around with this, and it does work if the element is renamed to, say,
notValue
, but I'm encountering an ambiguity with the treatment of thevalue
intrinsic. Would you be content with this if we pushed through the proposed change fromvalue
toxmlValue
as the labeling of the intrinsic?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.
(or even limiting the intrinsic to
""
)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.
""
and"xmlValue"
are both fine,"xmlValue"
(or everything that starts with "xml" for that matter) is not a valid element or attribute, at least according to my understanding of the XML standard: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.
On the other hand, isn't decoding the value of
<container />
element to an empty string is the expected behaviour here? I think I also relied on this in CoreXLSX in some of its behavior if I remember correctly, but this needs to be confirmed.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.
This might seem a bit self-centered, but if it doesn't break CoreXLSX, I'm probably fine with the change, hope MusicXML test suite would pass as well. Just wondering how many other apps and libraries we could break with this change and what would be possible workarounds in that case.
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.
Hoping we can perhaps circumvent these concerns by changing the use of the
"value"
coding key, per my latest PR! It does mean downstream users would have to introduce= ""
additions, but since this change is probably on the way anyway, I wonder if now is as good a time as any...?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.
@bwetherfield Will this test be added back?