Add ability to encode ProjectSpec to JSON #545
Conversation
| } | ||
|
|
||
| if targets.count > 0 { | ||
| dict["targets"] = targets |
There was a problem hiding this comment.
All these if array.count > 0 { are not really required. It might be useful for some use cases to generate out all the empty arrays. For the compact spec you have now, there could be a function JSONDictionary.removeEmptyArrays that loops through and removes all empty arrays
There was a problem hiding this comment.
Definitively a good approach generating empty template really help getting started.
| if let watchOS = watchOS { | ||
| dict["watchOS"] = watchOS.string | ||
| } | ||
| if let macOS = macOS { |
There was a problem hiding this comment.
All these if statements for optional values could be removed if the dictionary was [String: Any?].
|
|
||
| public protocol JSONDynamicEncodable { | ||
| // returns JSONDictionary or JSONArray or JSONRawType | ||
| func toJSONValue() -> Any |
There was a problem hiding this comment.
I'm not sure we need all these protocols as they aren't really used on their own. I think all the functions could just be the func toJSONValue() -> Any. Maybe just the top level spec would have a function that returned a dictionary.
| func testJSONEncodable() { | ||
| describe { | ||
| $0.it("encodes to json") { | ||
| let proj = Project(basePath: Path.current, |
There was a problem hiding this comment.
Nice object 😄 This might be useful for other tests
|
@yonaskolb Thank you for your reviewing! I fixed them up. As you said, It would be nice to add the process of removing nil and empty arrays / dictionaries in the process of generating JSON for human reading. |
| ] | ||
|
|
||
| if optional { | ||
| dict["optional"] = true |
There was a problem hiding this comment.
I'm a bit worried about the maintainability of all these properties, and that regressions might occur in the future by accident.
To aid with the properties that have defaults, and because we're now mapping those defaults in 3 places (property init, decoding, and now encoding), it would be good if we have static properties for all of these. In this example:
static let optionalDefault = falseThis this check would be
if optional != optionalDefault {There was a problem hiding this comment.
@yonaskolb It's make sense. Since I changed it, could you please review it?
There was a problem hiding this comment.
Looks good! Could you please rebase to fix the merge conflict and add a changelog?
| randomExecutionOrder = jsonDictionary.json(atKeyPath: "randomExecutionOrder") ?? false | ||
| parallelizable = jsonDictionary.json(atKeyPath: "parallelizable") ?? false | ||
| randomExecutionOrder = jsonDictionary.json(atKeyPath: "randomExecutionOrder") ?? Scheme.Test.TestTarget.randomExecutionOrderDefault | ||
| parallelizable = jsonDictionary.json(atKeyPath: "parallelizable") ?? Scheme.Test.TestTarget.parallelizableDefault |
There was a problem hiding this comment.
Very minor and doesn't need to change, but not sure you need to traverse the whole namespace path, I think you can just use TestTarget.parallelizableDefault
There was a problem hiding this comment.
Thanks!
After trying it out, it seems necessary to specify the complete namespace in the extension.
ryohey
force-pushed
the
projectspec-json
branch
from
d5645ea to
ea6fb60
Compare
|
@yonaskolb Thank you for your review! I rebased it and fix conflicts, added changelog. |
|
Thanks @ryohey! Not sure how you want to follow this PR up and how it will be used (you mentioned project migration), but I think it would be useful having a parameter like enum ProjectEncodingStyle {
/// removes empty dictionaries and arrays
case minimal
case full
}Feel free to do that here or in another PR. Perhaps would be better here |
|
@yonaskolb Thanks! I received a pull-request from @AndreiVidrasco that adds feature to filter out empty values from dictionary. and we talked about this pr #545. So I merge this pr this time. |
I first tried to implement spec generation, but as you mentioned, I needed to be able to serialize ProjectSpec. #9 (comment)
I defined
JSONDictionaryEncodableprotocol likeJSONObjectConvertibleof JSONUtilities, and implemented it in ProjectSpec and related classes. I did not use Codable because some classes do not have a simple correspondence with the JSON structure.I wrote a test that encodes ProjectSpec into JSON and decodes back and to test if they are equal. However, it may not be perfect because the tests do not cover all patterns and implementations are written by a lot of manual work.
fixes #123