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

Parse objects conforming the Codable protocol #99

Merged
merged 17 commits into from
Oct 7, 2017
Merged

Parse objects conforming the Codable protocol #99

merged 17 commits into from
Oct 7, 2017

Conversation

pepicrft
Copy link
Contributor

@pepicrft pepicrft commented Oct 2, 2017

Resolves #83

Short description 📝

Remove Unbox dependency by using the native Codable protocol.

Solution 📦

Update all the elements to conform the Codable protocol and remove the Unboxable conformance.

Implementation 👩‍💻👨‍💻

  • PBXFileElement
  • PBXBuildFile
  • PBXAggregateTarget
  • PBXBuildPhase
  • PBXContainerItemProxy
  • PBXCopyFilesBuildPhase
  • PBXFileReference
  • PBXFrameworksBuildPhase
  • PBXGroup
  • PBXHeadersBuildPhase
  • PBXNativeTarget
  • PBXProductType
  • PBXProj
  • PBXProject
  • PBXReferenceProxy
  • PBXResourcesBuildPhase
  • PBXShellScriptBuildPhase
  • PBXSourceTree
  • PBXSourcesBuildPhase
  • PBXTarget
  • PBXTargetDependency
  • PBXVariantGroup
  • XCBuildConfiguration
  • XCVersionGroup
  • XCWorkspace
  • XCWorkspaceData

GIF

gif

@pepicrft pepicrft self-assigned this Oct 2, 2017
@pepicrft pepicrft requested a review from a team October 2, 2017 07:27
@pepicrft pepicrft added this to the 0.4.0 milestone Oct 2, 2017
do {
_ = try PBXFileElement(reference: "ref", dictionary: dictionary)
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's probably worth making an extension on PBXObject or Decodable which is init(jsonDictionary: [String: Any] that handles all this boilerplate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah good idea, I'll do it @yonaskolb

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done @yonaskolb

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice! I think you could also put an extension on PBXObject as well that is an init with reference

init(reference: String, jsonDictionary: [String: Any]

That would remove a bit of the boilerplate in tests and other places

@artemnovichkov
Copy link
Contributor

artemnovichkov commented Oct 4, 2017

Can I help you with it? PBXBuildFile is next I guess

@pepicrft
Copy link
Contributor Author

pepicrft commented Oct 4, 2017

Sure @artemnovichkov, you are welcome to help. It'll take us time to migrate all of them, but with help, we'll be faster 😛

self.buildRules = try container.decode([String].self, forKey: .buildRules)
self.dependencies = try container.decode([String].self, forKey: .dependencies)
self.name = try container.decode(String.self, forKey: .name)
self.productName = try container.decode(String?.self, forKey: .productName)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Optional properties should use ‘decodeIfPresent’

self.name = try container.decodeIfPresent(String.self, forKey: .name)
self.sourceTree = try container.decode(PBXSourceTree.self, forKey: .sourceTree)
self.versionGroupType = try container.decode(String.self, forKey: .versionGroupType)
let children = try? container.decode([String].self, forKey: .children)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think lines like this can be:

self.children = try container.decodeIfPresent([String].self, forKey: .children) ?? []

@yonaskolb
Copy link
Collaborator

I find this extension really useful when using Decodable

extension KeyedDecodingContainer {

    public func decode<T>(_ key: KeyedDecodingContainer.Key) throws -> T where T: Decodable {
        return try decode(T.self, forKey: key)
    }

    public func decodeIfPresent<T>(_ key: KeyedDecodingContainer.Key) throws -> T? where T: Decodable {
        return try decodeIfPresent(T.self, forKey: key)
    }
}

It makes the code easier to write as you don't need to specify the type again:

- self.name = try container.decode(String.self, forKey: .name)
+ self.name = try container.decode(.name)

It also means that support can be added by doing search and replace on the unbox functions

@pepicrft
Copy link
Contributor Author

pepicrft commented Oct 5, 2017

It's very useful! Great idea @yonaskolb. I'll update the code to use it.

@pepicrft pepicrft merged commit 59bbf44 into master Oct 7, 2017
@pepicrft pepicrft deleted the codable branch October 7, 2017 12:53
@yonaskolb
Copy link
Collaborator

I just noticed that this removed the BuildSettings typealias. Was there any particular reason for that?

@pepicrft
Copy link
Contributor Author

I came across some issues trying to parse the settings, and I removed it thinking it was related. It turned out it wasn't. Do you think we should keep it?

@yonaskolb
Copy link
Collaborator

It’s not strictly needed, but I think it was nice how it unified the types and purpose wherever build settings were used. The factthat it was public means people may have been using it too (as was the case of XcodeGen)

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.

3 participants