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

Attributed Intrinsic (value coding key) #73

Merged
merged 28 commits into from Feb 22, 2019

Conversation

JoeMatt
Copy link
Contributor

@JoeMatt JoeMatt commented Feb 6, 2019

Overview

This PR fixes #12: decoding of unkeyed single value elements that contain attributes as reported in that issue.

Example

<?xml version="1.0" encoding="UTF-8"?>
<foo id="123">456</foo>
private struct Foo: Codable, DynamicNodeEncoding {
    let id: String
    let value: String

    enum CodingKeys: String, CodingKey {
        case id
        case value
        // case value = "" would also work
    }

    static func nodeEncoding(forKey key: CodingKey) -> XMLEncoder.NodeEncoding {
        switch key {
        case CodingKeys.id:
            return .attribute
        default:
            return .element
        }
    }
}

Previously this XML example would fail to decode. This PR allows two different methods of decoding discussed in usage.

Usage

This PR will support decoding the example XML in two cases as long as the prerequisite cases are matched.

Prerequisites

  1. No keyed child elements exist
  2. Any keyed child nodes are supported Attribute types and are indicated to decode as such

Supported cases

  1. An instance var with the key value of any decodable type.
  2. An instance var of any key that has a Decoding key of String value "value" or "".

The decoder will look for the case where an element was keyed with either "value" or "", but not both, and only one of those values (ie; no other keyed elements). It will automatically find the correct value based on the CodingKey supplied.

Other considerations

The choice to decode as either "value" or "" keys was purely to try to support the inverse to XML version which would only work if an instance var specifically has a CodingKey with associated value type String that returns an empty string, if PR #70 is commited as-is, which adds XML coding support for unkeyed attributed value elements.

The 'value' variant was added as a simpler means to support decoding a nested unkeyed element without having to provide custom CodingKey enum for a struct. Something needed to be provided since Swift doesn't have empty string iVars let "" : String, isn't a valid iVar token for example, so value was chosen as a logical default.

Notes

This PR is an extension of #70 , though it could be recoded to work off of master. The last commit in this PR is the only commit specific to this feature, though #70 provides the inverse solution of creating XML from an attributed value wrapping struct.

Coding and decoding unit tests of String and Int values are included.

…percased

Some helpers to deal with converting auto-generated codable String values for instance variables to match some common XML key coding standards to the commonly used Swift camel casing

- capitalzied: convert first letter to uppercase
- uppercased: All letters uppercased
- lowercased: All letters lowercased

Support all types that conform to StringProtocol rather than just String
Use a type erased protocl inheritance strategy commonly used to provide default implimentation to avaoid issues with as? checks in generic protocols, while retaining reuse benefits of generic protocols
…y string

In the case where a codable provides an empty string for the codable string value for an instance variable an empty bracket was inserted which is invalid XML.

```
let attr = "bar"
let value = "FOO"
enum CodingKeys : String, CodingKey {
    case attr
    case value = ""
}
```

Will be useful for unkeyed objects that contain only attributes eg;
```xml
<box attr="bar"><>FOO</></box>
<!-- Would now correctly become -->
<box attr="bar">FOO</box>
```
DynamicNodeEncoding allows easily adding the ability to choose if iVars should be attribute or element encoded by inheriting DynamicNodeEncoding and implimenting a single static function in any Codable class or struct.

This is simpler than the current method that requires a global dynamic encoding closure for every XMLEncoder instance. This allows changing the encoding where the data models live, rather than the creator of the XMLEncoder instance needing to have knowledge of all the possible structs and classes that the encoder might encounter at init time.
- refactor element and attribute encoders to closures for easier code reuse
- Added type alias for encoding closures for clarity
Had removed them since I was testing intrinsics with attributes. Needed to wrap cateogy value in an element tag again.

Also appears the hack for decoding nested arrays is no longer required so removed the complex decoding of Category
Previous version of this test techncially passed on Encdode/Decode comparision sinve the structure values were the same, but the encoding make Book structs id an element, so the strings weren't equal.

Modified the simplier single book test to check that the attributes are encoded to XML and match the original string (minus white space formatting)
Add intrinsic encoding decoding with attributes support
@JoeMatt JoeMatt force-pushed the feature/12-attributedIntrinsic branch from 94b841f to 559f35a Compare February 6, 2019 06:36
@JoeMatt
Copy link
Contributor Author

JoeMatt commented Feb 6, 2019

It's incredibly difficult to find the failing error i the Travis logs. I'd suggest turning off the verbosity, and if linting is going to be a failure it might be faster to incorporate it as a pre-commit hook or use Danger or something that's faster than a full travis build for non-compilation related failures.

@MaxDesiatov MaxDesiatov self-assigned this Feb 9, 2019
@MaxDesiatov
Copy link
Collaborator

MaxDesiatov commented Feb 9, 2019

Thanks @JoeMatt, I've fixed SwiftFormat complaints and also merged master into this after #70 was merged. KeyedTests and UnkeyedTests are still failing though.

@JoeMatt
Copy link
Contributor Author

JoeMatt commented Feb 11, 2019

@MaxDesiatov Interesting, I don't think they were failing on my branch. I'll try to take a look this week. I'm moving this code into my own SOAP Swift Coding library that depends on XMLCoder so I'll have to fix any issues that arise. I'll push up fixes as I find them.

@MaxDesiatov
Copy link
Collaborator

MaxDesiatov commented Feb 12, 2019

@JoeMatt while I've tried to remove that DynamicNodeEncoding extension in one of the commits, I did have to add it back to make all of the tests succeed before merging #70. The extension is also available in the corresponding branch of this PR, so doesn't look like that's the reason

@thecb4
Copy link

thecb4 commented Feb 13, 2019

Any news here? I'm definitely interesting in using this to support AppStore Connect uploading.

@JoeMatt
Copy link
Contributor Author

JoeMatt commented Feb 13, 2019

A few days I’ll be back on this. Need to fix something in another dependency of my app first.

@Eke
Copy link

Eke commented Feb 19, 2019

Hello, any updates on this?

@JoeMatt
Copy link
Contributor Author

JoeMatt commented Feb 19, 2019

I'm testing / fixing a case where the attributes are enums with associated values that can be reduced to a simple box.

Also, it's ready when it's ready, no need to keep posting for updates. Subscribe to the issue to get notified on updates.

@MaxDesiatov
Copy link
Collaborator

Thanks @JoeMatt, no worries! I was thinking if I should pick up the branch myself, I wonder if any of the commenters were interested in contributing or planning their own work around this given that it's a pressing issue. Please let me know if there is any way I can help.

@MaxDesiatov
Copy link
Collaborator

hm, most of the linter warnings reported by @houndci-bot are not valid, it probably didn't pick up the actual config. Sorry about that, it was setup just recently. I'll resolve irrelevant warnings

@MaxDesiatov
Copy link
Collaborator

also, merged master, maybe that'll make it pick up the correct config

@codecov
Copy link

codecov bot commented Feb 21, 2019

Codecov Report

Merging #73 into master will increase coverage by 0.24%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #73      +/-   ##
==========================================
+ Coverage    74.4%   74.65%   +0.24%     
==========================================
  Files          37       37              
  Lines        1735     1752      +17     
==========================================
+ Hits         1291     1308      +17     
  Misses        444      444
Impacted Files Coverage Δ
Sources/XMLCoder/Auxiliaries/XMLCoderElement.swift 95.34% <100%> (+0.11%) ⬆️
...s/XMLCoder/Decoder/XMLKeyedDecodingContainer.swift 83.23% <100%> (+0.62%) ⬆️
Sources/XMLCoder/Auxiliaries/Box/KeyedBox.swift 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eafcdf1...7a4f94f. Read the comment docs.

@JoeMatt
Copy link
Contributor Author

JoeMatt commented Feb 21, 2019

@MaxDesiatov I have one more thing I'm working on. Doesn't need to be in this PR.
It's when an enum is the attribute that should be decoded as a simple type, string etc.

example:

    <number type="unit">A201</number>
   public enum PostingType : Equatable, Codable {
        private enum CodingKeys: String, CodingKey {
            case unit
            case resno
        }

        public init(from decoder: Decoder) throws {
            let values = try decoder.container(keyedBy: CodingKeys.self)
            if let unit = try values.decodeIfPresent(String.self, forKey: .unit) {
                self = .unit(unit)
                return
            } else {
                let resno = try values.decode(String.self, forKey: .resno)
                self = .resno(resno)
                return
            }
        }

        public func encode(to encoder: Encoder) throws {
            var container = encoder.container(keyedBy: CodingKeys.self)
            switch self {
            case .unit(let unit):
                try container.encode(unit, forKey: .unit)
            case .resno(let resno):
                try container.encode(resno, forKey: .resno)
            }
        }

        case unit(String)
        case resno(String)
    }
    
    public struct PostingNumber : VendorDataProtocol, DynamicNodeEncoding {

        public let type : PostingType

        public init(type: PostingType) {
            self.type = type
        }

        enum CodingKeys : String, CodingKey {
            case type
            case typeValue = ""
        }

        public static func nodeEncoding(forKey key: CodingKey) -> XMLEncoder.NodeEncoding {
            switch key {
            case PostingNumber.CodingKeys.type: return .attribute
            default: return .element
            }
        }

        public init(from decoder: Decoder) throws {
            let container = try decoder.container(keyedBy: CodingKeys.self)

            let type = try container.decodeIfPresent(PostingType.self, forKey: .type)
            let typeArray = try container.decodeIfPresent([PostingType].self, forKey: .type) // Deal with the double value of type as attribute and element
            if let type = type {
                self.type = type
            } else if let typeArray = typeArray, let first = typeArray.first {
                self.type = first
            } else {
                throw DecodingError.keyNotFound(CodingKeys.type, DecodingError.Context.init(codingPath: decoder.codingPath, debugDescription: "Missing type"))
            }
        }

        public func encode(to encoder: Encoder) throws {
            var container = encoder.container(keyedBy: CodingKeys.self)
            switch type {
            case .unit(let unit):
                try container.encode("unit", forKey: .type)
                try container.encode(unit, forKey: .typeValue)
            case .resno(let resno):
                try container.encode("resno", forKey: .type)
                try container.encode(resno, forKey: .typeValue)
            }
        }
    }

@MaxDesiatov
Copy link
Collaborator

MaxDesiatov commented Feb 21, 2019

looks like all checks pass, thank you very much @JoeMatt! I'll review this in the next few days

}
}

// TODO: Fix decoding

Choose a reason for hiding this comment

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

Todo Violation: TODOs should be resolved (Fix decoding). (todo)

@JoeMatt
Copy link
Contributor Author

JoeMatt commented Feb 22, 2019

I pushed one more commit which is the codable test for associated type unkeyed intrinsics.

The decoding I couldn't get to work. Requires more knowledge of the decoding code. I tried adding deifference decoders with code like

    func unbox<T: Decodable & RawRepresentable>(_ box: Box) throws -> T where T.RawValue: Decodable { ...
T.init(rawValue: 
}

No dice, and that only covers enums that are RawRepresentable. Associated value types I have no idea how to do or if it's even possible in Swift 4.2.

Copy link
Collaborator

@MaxDesiatov MaxDesiatov left a comment

Choose a reason for hiding this comment

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

Thanks again, LGTM in general! We try to avoid print calls in tests as those slow down test execution and you can attach printing breakpoints during debugging anyway. I also think we don't need to catch errors in tests explicitly as those are rethrown and reported automatically by XCTest. I've added those changes myself to quickly merge the PR as I see a few people would find it handy merged sooner.

Looking forward to the DynamicNodeDecoding PR if there's anything like that in the works.

@MaxDesiatov MaxDesiatov merged commit 011f493 into CoreOffice:master Feb 22, 2019
@khoogheem khoogheem mentioned this pull request Feb 22, 2019
@JoeMatt
Copy link
Contributor Author

JoeMatt commented Feb 22, 2019

Cool, thanks a lot @MaxDesiatov .

Sorry for leaving in those prints. I have a habit of putting them everywhere in my unit tests but in my local setup, they get shuffled to a console logging app.

XMLCoder is really coming along!

If I see anything else in my code that could be of use I'll definitely make more PRs.

@MaxDesiatov MaxDesiatov changed the title Attributed Intrinsic Attributed Intrinsic (value coding key) Mar 24, 2019
MaxDesiatov pushed a commit that referenced this pull request Nov 11, 2019
## Overview
Restricts the special case usage of `"value"` OR `""` as a coding key, as implemented in #73 to just `""`. `"value"` resumes functioning as a normal coding key. 

## Example
As noted in #145, `"value"` is starting to collide awkwardly in one or two cases where it may be _implicit_ per the special case usage 
```swift
<value-containing>I am implicitly labeled</value-containing>
``` 
or _explicit_, as in 
```swift
<value-containing>
    <value>I am explicitly labeled</value>
</value-containing>
``` 
With the change proposed, the latter example will be unambiguously captured by
```swift
struct ValueContaining: Codable {
    let value: String
}
```
while the former is equivalent to
```swift
struct ValueContaining: Codable {
    let value: String

    enum CodingKeys: String, CodingKey {
        case value = ""
    }
}
```
## Source Compatability

This is a **breaking change** for downstream users of the special `"value"` coding key. All such uses should be replaced by `case value = ""` subject to this PR's inclusion.
arjungupta0107 pushed a commit to salido/XMLCoder that referenced this pull request Jun 26, 2020
## Overview
Restricts the special case usage of `"value"` OR `""` as a coding key, as implemented in CoreOffice#73 to just `""`. `"value"` resumes functioning as a normal coding key. 

## Example
As noted in CoreOffice#145, `"value"` is starting to collide awkwardly in one or two cases where it may be _implicit_ per the special case usage 
```swift
<value-containing>I am implicitly labeled</value-containing>
``` 
or _explicit_, as in 
```swift
<value-containing>
    <value>I am explicitly labeled</value>
</value-containing>
``` 
With the change proposed, the latter example will be unambiguously captured by
```swift
struct ValueContaining: Codable {
    let value: String
}
```
while the former is equivalent to
```swift
struct ValueContaining: Codable {
    let value: String

    enum CodingKeys: String, CodingKey {
        case value = ""
    }
}
```
## Source Compatability

This is a **breaking change** for downstream users of the special `"value"` coding key. All such uses should be replaced by `case value = ""` subject to this PR's inclusion.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can’t reach an XML value
5 participants