JSON parsing and factories #7

joanromano · 2017-01-13T10:36:22Z

Closes #3

First approach to JSON parsing and factories.

JSONFactory wraps registered factory builders (closures which provide Components) and uses them in order to parse JSONObjects and create the main Component

Still to do:

Update README documentation, after agreement on the approach. We should also add here a proper documentation on the JSON schema we follow.

@MP0w @mathiasAichinger @AndreasThenn let's do this!

MOD changed SwiftLint line length warning to 150

…rsion uses 120 per default

joanromano · 2017-01-13T10:41:09Z

Tests/JSONFactoryTests.swift

+            return children
+        }
+    }
+}


Maybe we should add this as a public method on Component?

I think it shouldn't be needed (so shouldn't be exposed) but maybe I'm missing the point to use it?

I will keep it there for now

codecov-io · 2017-01-13T10:44:46Z

Current coverage is 99.16% (diff: 100%)

Merging #7 into master will increase coverage by 0.10%

@@             master         #7   diff @@
==========================================
  Files             7          8     +1   
  Lines           214        240    +26   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits            212        238    +26   
  Misses            2          2          
  Partials          0          0

Powered by Codecov. Last update 660b435...6a9d36a

AndreasThenn · 2017-01-13T12:09:44Z

I think we should update the code (JSONReaderTests, app-structure.json, ..) first, so that it fits to our decision to get rid of the id for now.

joanromano · 2017-01-13T12:18:35Z

@AndreasThenn tests are already updated to check if we assert when no type. I am going to update the app-structure.json to reflect our current schema though, yes.

mathiasAichinger · 2017-01-13T14:39:23Z

@joanromano JSONFactory, StackClusterFactory, TabBarClusterFactory cannot be intialized from outside the module because they have no public initializer.

mathiasAichinger · 2017-01-13T14:57:02Z

App Example can be found in feature/jsonParsingExample. Will create a PR after this is finished.

joanromano · 2017-01-13T17:43:38Z

@mathiasAichinger good catch, fixing and updating

MP0w · 2017-01-13T17:55:57Z

Tests/JSONFactoryTests.swift

+                let faultyProduceNoKeys = { _ = try! jsonFactory.produce(from: ["foo": "bar"]) }
+                let faultyProduceNoType = { _ = try! jsonFactory.produce(from: ["id": "bar"]) }
+
+                expect(faultyProduceNoKeys()).to(throwAssertion())


should not throw an assertion. throws an error. However it's now asserting because you are try!ing

true, forgot to update after updating error handling

MP0w · 2017-01-13T17:56:24Z

Tests/ComponentTests.swift

@@ -15,7 +15,7 @@ class ComponentTests: QuickSpec {

    override func spec() {

-        typealias DictMeta = Dictionary<String, String>
+        typealias DictMeta = [String:String]


good but missing a space after :

MP0w · 2017-01-13T17:57:46Z

Tests/StackClusterTests.swift

@@ -112,7 +112,7 @@ class StackClusterTests: QuickSpec {
                    labelComponent(title: "1", color: .red, labelSize: size),
                    labelComponent(title: "2", color: .red, labelSize: size),
                    labelComponent(title: "3", color: .red, labelSize: size),
-                    labelComponent(title: "4", color: .red, labelSize: size),
+                    labelComponent(title: "4", color: .red, labelSize: size)


I like trailing comma in collections because you can easily add or move lines... I know swiftlint complains with recent versions but we might disable the rule eventually

I actually agree with swiftlint here :D

I am fine with both solutions, but I would choose the one that differs not from the default ruleset, so we can avoid custom rules wherever it is possible.

MP0w · 2017-01-13T18:00:06Z

Source/Factories/ComponentFactory.swift

+    ///   - children: an array of children component
+    ///   - meta: a ComponentMeta object
+    /// - Returns: An optional Component
+    func produce(children: [Component],


not a fan of this naming... produce...

Suggestions?

MP0w · 2017-01-13T18:04:17Z

Source/Factories/StackClusterFactory.swift

+
+    /// A concrete ComponentFactory which produces a stack cluster component
+    public final class StackClusterFactory: ComponentFactory {
+        public func produce(children: [Component],


it seems to me that these factories have something wrong.
To comform to ClusterFactory you need a function that given children and meta creates a Component
then ClusterLayout.stack... ClusterLayout.tab... are already factories so we are creating all this code to basically wrap those functions.

Other than this this factory implementation seems wrong to me because it's only working for .cluster
How do you create wrapper and view from json? Am I missing anything?

in the proof of concept implementation I had 3 factories (one for cluster one for wrapper one for view) because each type has different inputs.
Also all this XXXFactory seems not need to me since all we want is a function that given the correct input returns a Component

Factories are meant to provide the type to register. Maybe we can skip that and just have JSONFactory registering the functions for a given string, but I liked the idea of representing a factory also.

In the tests I create factories for clusters and views, and I guess would also work for wrappers right?

So if I am not wrong the difference between this and the proof of concept is that here we are more explicit on the factories (which also needs more code, true) and in the proof of concept the factory only registers closures for given types.

MP0w · 2017-01-13T18:09:32Z

Source/JSON/JSONFactory.swift

+        let children = json[childrenKey] as? [JSONObject] ?? []
+        let componentChildren = try children.flatMap { try produce(from: $0) }
+
+        return factories[type]?.produce(children: componentChildren, meta: meta)


yes looks like only cluster can be created from json

In the factory tests we create clusters or plain views, right?

Well you can by ignoring the children (empty array) , the view shouldn't have children.
And what about wrappers? They don't have children but one child

You would get the first child of the children. So for you what's wrong here is having same factory API for the three kind of Component's

Yes and actually you don't need three factory (will write some pseudo code when I arrive at home)
Having to ignore children or get the first seems bad API design.
Other than this, defining types that wraps functions (e.g. StackFactory) makes registration way more verbose than needed

Trying out that approach too, let's see what we each get

Got something out of what you said, I will update the tests now and we can discuss the differences

I wrote some pseudocode:

typealias JSONRepr = [String: Any] fileprivate protocol FactoryBuilder { var factory: (JSONRepr) -> Component { get } } fileprivate class ClusterFactoryBuilder: FactoryBuilder { let factory: (JSONRepr) -> Component init(builder: @escaping Component.ClusterBuilder) { self.factory = { (json) in return Component.cluster( builder: builder, children: json["children"] as! [Component], meta: json["meta"] as! [String: Any] ) // pseudo..... } } } // same for.... // fileprivate class WrapperFactoryBuilder: FactoryBuilder // fileprivate class ViewFactoryBuilder: FactoryBuilder class ComponentFactory { private static var factories: [String: FactoryBuilder] = [:] class func registerCluster(for id: String, builder: @escaping Component.ClusterBuilder) { factories[id] = ClusterFactoryBuilder(builder: builder) } class func registerWrapper(for id: String, builder: Component.WrapperBuilder) { // factories[id] = WrapperFactoryBuilder(builder: builder) } class func registerView(for id: String, builder: Component.ViewBuilder) { // factories[id] = ViewFactoryBuilder(builder: builder) } class func component(for json: JSONRepr) -> Component? { let id = json["id"] as! String // pseudo return factories[id]?.factory(json) } } // USAGE ComponentFactory.registerView(for: "myView") { (meta) -> UIViewController? in return UIViewController() } ComponentFactory.registerCluster(for: "stack") { (children, meta) -> UIViewController? in return StackViewController() } ComponentFactory.registerCluster(for: "tabvar", builder: tabBarBuilder(Bundle.main))

Got something similar with minor differences, i will push and we can check

I like this approach now, because it's using closures everywhere. The first approach was a little bit a mixture of the closure and class approach. I still prefer the class approach, but probably because I'm thinking in old concepts ;) (I would have used classes for the clusters and protocols for the factories.)

joanromano · 2017-01-13T20:16:27Z

@MP0w refactored following a similar approach, doing the json handling within the json factory.

Also using the factory as an instance with instance methods instead of static methods with a single static builder registration: still not sure which is the best solution there.

MP0w · 2017-01-13T22:52:07Z

Source/JSON/JSONReader.swift

+    ///   - bundle: the bundle where the file is located
+    /// - Returns: an optional serialized JSONObject
+    /// - Throws: throws an error in case of failure or invalid JSON data
+    public class func jsonObject(from jsonFilename: String, bundle: Bundle = .main) throws -> JSONObject? {


this shouldn't be part of Matrioska, I think the whole JSONReadershouldn't be part of Matrioska since it's just convenience functions for JSONSerialization and retrieving a file

Let's move this class to the test target for now. If we need it later on we can move it again to the project.

Is still in Source, should be in Test/

Didn't apply any changes yet, was waiting for the full review

MP0w · 2017-01-13T22:53:35Z

Tests/.swiftlint.yml


 opt_in_rules:
  - empty_count
-  - force_unwrapping


why removed all these rules?

This is the swiftlint for tests so I disabled them only here.

joanromano · 2017-01-13T23:06:00Z

Overall my feelings:

This second approach is a bit more handling on our side but better and easier usage for clients: just registering closures into the json factory and having always three kind of factory builders.
In the other hand the first approach let's you express better what a factory is for and having different factories in code, but requires more work on the client side and has a single api for factory building which one may argue that is not 100% correct.

I would go for the ease of use of the api then and keep the second approach.

MP0w

Didn't properly check the test because I think we still need to define the json schema properly

MP0w · 2017-01-15T22:38:06Z

Example/MatrioskaExample/TileViewController.swift

@@ -9,7 +9,7 @@
 import UIKit
 import Matrioska

-struct TileConfig: MaterializableComponentMeta {
+struct TileConfig: ExpressibleByComponentMeta {


Maybe setup buddybuild quickly so we can make sure to not break the example

Done (wanted to see how it works), I created a runtastic team on buddybuild. Will invite you guys. Btw I think buddybuild also supports libraries now?

Oh nevermind that's only for payed plans.

MP0w · 2017-01-15T22:40:52Z

Source/Factories/JSONFactory.swift

+
+    static let typeKey = "type"
+    static let metaKey = "meta"
+    static let childrenKey = "children"


I think we need to son specs and documentation (schema would also be nice)... because I think we are missing child for wrapperfor example

For now we agreed on having the same children key for both wrapper and cluster afaik

Yep, as said at the beginning of the PR the documentation is still to do.

Don't agree with using children instead of child. If we want to have wrappers (and I think we want) they shouldn't be an hacky cluster (a cluster that ignore children other than the first.
Please couldn't you guys reconsider this decision or try to explain why you think it should be like this?

I think this is a discussion for #3 probably. Anyway nobody said we don't want to have wrappers, it's just the agreement on the json schema.

MP0w · 2017-01-15T22:42:45Z

Source/Factories/JSONFactory.swift

+/// A factory that wraps ComponentFactory objects and uses them to produce Components
+public final class JSONFactory {
+
+    static let typeKey = "type"


all these properties should be private...
Also consider a nested enum for namespacing:

enum Keys { // not a struct because an enum with no cases can't be initialized static let type = "type" }

Sure, but what would the nested enum namespacing bring if we make it private?

Usage would be Keys.type instead of JSONFactory.type .
More explicit imho

I also thought it might be good for these keys to be public so that clients now which keys to expect. That could also be reflected on the documentation though.

MP0w · 2017-01-15T22:45:24Z

Source/Factories/JSONFactory.swift

+        let componentChildren = try children.flatMap { try component(from: $0) }
+        var componentResult: Component?
+
+        if let viewFactory = viewFactory[type] {


I would do the matching differently:

let id = .... if let children = ... { // is optional // then is a cluster } else if let child = ... { // then is a wrapper } else { // must be a view }

But this depends by the json specs

For now we would have the same key for children in the json

MP0w · 2017-01-15T22:45:51Z

Source/JSON/JSONReader.swift

+    ///   - bundle: the bundle where the file is located
+    /// - Returns: an optional serialized JSONObject
+    /// - Throws: throws an error in case of failure or invalid JSON data
+    public class func jsonObject(from jsonFilename: String, bundle: Bundle = .main) throws -> JSONObject? {


Is still in Source, should be in Test/

MP0w · 2017-01-15T22:46:06Z

Tests/.swiftlint.yml


 opt_in_rules:
  - empty_count
-  - force_unwrapping


MP0w · 2017-01-15T22:48:51Z

Tests/JSONFactoryTests.swift

+            return children
+        }
+    }
+}


I think it shouldn't be needed (so shouldn't be exposed) but maybe I'm missing the point to use it?

mathiasAichinger · 2017-01-17T13:59:43Z

Source/Factories/JSONFactory.swift

+    /// - Throws: `JSONFactoryError` when a mandatory key is missing
+    public func component(from json: JSONObject) throws -> Component? {
+        guard let type = json[Key.type] as? String else {
+            throw JSONFactoryError.missing(json, Key.type)


I think there should be a possibility to register fallback components. So eg. for clusters always fallback to stacks.

I mentioned this on last meeting and they said we don't include fallback components for now? We could implement it here though.

joanromano · 2017-01-18T12:07:31Z

	1 Warning
⚠️	Big PR

SwiftLint found issues

Warnings

File	Line	Reason
ComponentMeta.swift	78	Line should be 100 characters or less: currently 103 characters

Generated by 🚫 danger

JSON parsing and factories

Mathias Aichinger and others added 11 commits January 11, 2017 17:10

ADD first draft for component factories. Not happy yet; WIP

823cb16

ADD JSONReader to read a json file into a dictionary (JSONObject)

6eb292f

MOD changed SwiftLint line length warning to 150

ADD unit tests for JSONReader

8c6bc88

REM line_length setting for SwiftLint again, because new SwiftLint ve…

ba91bc7

…rsion uses 120 per default

Add JSONFactory wrapper

6ecf8d0

Update swiftlint tests file

77b4626

Add tests to JSONReader

df43352

Add JSONFactoryError for JSONFactory error handling

d6c16f6

Minor header fixes

2a4fd05

Fix swift lint rules with new version

be460f6

Update project set up

f524260

joanromano commented Jan 13, 2017

View reviewed changes

Update app_structure.json and tests to reflect current schema

26acb59

MP0w suggested changes Jan 13, 2017

View reviewed changes

MP0w reviewed Jan 13, 2017

View reviewed changes

Refactor using a single JSONFactory which holds builder closures

62ca01d

joanromano added 5 commits January 13, 2017 21:18

Remove swiftlint line length since we use the default

06b4bcd

Add public init in JSONFactory

d8ff008

Merge branch 'master' into feature/jsonParsing

98a5b1d

Fix code in example

d218084

Fix line length violation on ExpressibleByComponentMeta documentation

81a720f

MP0w reviewed Jan 13, 2017

View reviewed changes

MP0w reviewed Jan 15, 2017

View reviewed changes

joanromano mentioned this pull request Jan 15, 2017

Load Components from JSON #3

Closed

3 tasks

joanromano added 2 commits January 16, 2017 16:45

CRC

b3cc35b

Use JSONObject type when possible

e030bf4

mathiasAichinger reviewed Jan 17, 2017

View reviewed changes

joanromano and others added 5 commits January 18, 2017 11:07

Re build example

dadde5d

Added gemfile to fix buddybuild

ae229e1

Added another gemfile

452a56b

ADD danger to the gemfile

50dcde2

Add danger-swiftlint

abd0a79

AndreasThenn force-pushed the feature/jsonParsing branch from 07b8514 to abd0a79 Compare January 19, 2017 08:56

joanromano added 5 commits January 19, 2017 14:33

Update documentation

c80a885

Update code and documentation with snake case keys

57b6d0b

Remove fast test

8119b1b

Update json example with snake case

6a9d36a

Fix link on Readme

63a84b9

mathiasAichinger approved these changes Jan 19, 2017

View reviewed changes

joanromano merged commit 792a2e4 into master Jan 19, 2017

joanromano deleted the feature/jsonParsing branch January 19, 2017 21:06

joanromano added a commit that referenced this pull request Jan 19, 2017

Merge pull request #7 from runtastic/feature/jsonParsing

759b0de

JSON parsing and factories

JSON parsing and factories #7

JSON parsing and factories #7

Conversation

joanromano commented Jan 13, 2017 • edited

joanromano Jan 13, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jan 13, 2017 • edited

Current coverage is 99.16% (diff: 100%)

AndreasThenn commented Jan 13, 2017

joanromano commented Jan 13, 2017

mathiasAichinger commented Jan 13, 2017

mathiasAichinger commented Jan 13, 2017

joanromano commented Jan 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndreasThenn Jan 17, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MP0w Jan 13, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joanromano commented Jan 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joanromano commented Jan 13, 2017

MP0w left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joanromano commented Jan 18, 2017 • edited

SwiftLint found issues

Warnings

joanromano commented Jan 13, 2017 •

edited

joanromano Jan 13, 2017 •

edited

codecov-io commented Jan 13, 2017 •

edited

AndreasThenn Jan 17, 2017 •

edited

MP0w Jan 13, 2017 •

edited

joanromano commented Jan 18, 2017 •

edited