Add a syntactic "Add Codable structs from JSON" code action #1205

DougGregor · 2024-04-26T00:05:50Z

Add a syntactic action that takes JSON pasted into a Swift file or
placed in a string literal, then turns it into a set of Codable
structs that can represent the JSON. Our typical example starts like
this:

{
    "name": "Produce",
    "shelves": [
        {
            "name": "Discount Produce",
            "product": {
                "name": "Banana",
                "points": 200,
                "description": "A banana that's perfectly ripe."
            }
        }
    ]
}

and turns into this:

struct JSONValue: Codable {
    var name: String
    var shelves: [Shelves]

    struct Shelves: Codable {
        var name: String
        var product: Product

        struct Product: Codable {
            var description: String
            var name: String
            var points: Double
        }
    }
}

The refactoring itself would live down in the swift-syntax package if
not for its dependency on Foundation. We'll move as appropriate.

DougGregor · 2024-04-26T00:06:00Z

@swift-ci please test

DougGregor · 2024-04-26T01:52:29Z

@swift-ci please test

DougGregor · 2024-04-26T01:53:06Z

@swift-ci please test

DougGregor · 2024-04-26T02:17:31Z

@swift-ci please test

DougGregor · 2024-04-26T02:17:52Z

@swift-ci please test Windows

DougGregor · 2024-04-26T02:46:33Z

@swift-ci please test Windows

DougGregor · 2024-04-26T15:53:26Z

@swift-ci please test Linux

DougGregor · 2024-04-26T17:32:53Z

@swift-ci please test Linux

ahoppen

Copied a few review comments from #1169 😉

Sources/SourceKitLSP/Swift/CodeActions/ConvertJSONToCodableStruct.swift

ahoppen · 2024-04-26T22:14:46Z

Sources/SourceKitLSP/Swift/CodeActions/ConvertJSONToCodableStruct.swift

+}
+
+extension ConvertJSONToCodableStruct {


Let’s just shove them all together instead of having extensions of the same type within the same file.

Suggested change

}

extension ConvertJSONToCodableStruct {

Same once more a few lines below.

I want the protocol-conforming extensions separate, but other than that---sure

This is no longer a protocol-conforming extension 😜 Also, I’m not generally a fan of putting protocol conformances in extensions. I think it makes it harder to reason about which protocols a type conforms to.

Sources/SourceKitLSP/Swift/CodeActions/ConvertJSONToCodableStruct.swift

ahoppen · 2024-04-26T22:19:46Z

Sources/SourceKitLSP/Swift/CodeActions/ConvertJSONToCodableStruct.swift

+}
+
+extension Array where Element == Any {
+  fileprivate func unwrap(_ depth: Int) -> Dictionary<String, Any>? {


Could you add a comment of what unwrap does?

ahoppen · 2024-04-26T22:21:07Z

Tests/SourceKitLSPTests/CodeActionTests.swift

+    let uri = DocumentURI.for(.swift)
+    let positions = testClient.openDocument(
+      """
+         1️⃣{


Nitpick

Suggested change

1️⃣{

1️⃣{

Also, 2 spaces indentation would be nice here for consistency.

ahoppen · 2024-04-26T22:24:54Z

Tests/SourceKitLSPTests/SyntaxRefactorTests.swift

+public func assertStringsEqualWithDiff(
+  _ actual: String,
+  _ expected: String,
+  _ message: String = "",
+  additionalInfo: @autoclosure () -> String? = nil,
+  file: StaticString = #filePath,
+  line: UInt = #line
+) {


If we’re adding the string diff functions, could you put them in SKTestSupport so that other tests can also use them?

ahoppen · 2024-04-26T22:25:13Z

Tests/SourceKitLSPTests/SyntaxRefactorTests.swift

+
+  // Use `CollectionDifference` on supported platforms to get `diff`-like line-based output. On
+  // older platforms, fall back to simple string comparison.
+  if #available(macOS 10.15, *) {


The #available check isn’t needed since sourcekit-lsp targets macOS 13.

Add a syntactic action that takes JSON pasted into a Swift file or placed in a string literal, then turns it into a set of Codable structs that can represent the JSON. Our typical example starts like this: ``` { "name": "Produce", "shelves": [ { "name": "Discount Produce", "product": { "name": "Banana", "points": 200, "description": "A banana that's perfectly ripe." } } ] } ``` and turns into this: ```swift struct JSONValue: Codable { var name: String var shelves: [Shelves] struct Shelves: Codable { var name: String var product: Product struct Product: Codable { var description: String var name: String var points: Double } } } ``` When converting to JSON, we attempt to reason about multiple JSON objects on the same level to detect when there are optional fields, due to either an explicit null or due to the absence of fields in some of the JSON objects that are conceptually stored together. The refactoring itself would live down in the swift-syntax package if not for its dependency on Foundation. We'll move it when appropriate.

DougGregor · 2024-05-06T20:58:41Z

@swift-ci please test

DougGregor · 2024-05-06T20:58:51Z

@swift-ci please test Windows

DougGregor · 2024-05-06T21:01:20Z

I just went ahead and rewrote this with a completely new structure that does merging of like elements in the whole tree. When merging a field w/ a missing value (or one explicitly marked null), we introduce an optional in Swift. This also handles Boolean values specially.

DougGregor · 2024-05-06T21:33:27Z

@swift-ci please test

DougGregor · 2024-05-06T21:33:36Z

@swift-ci please test Windows

ahoppen

That’s a lot shorter and simpler and even implements merging of structs 🤩

I have a few, mostly nitpicky, comments.

Sources/SourceKitLSP/Swift/CodeActions/ConvertJSONToCodableStruct.swift

ahoppen · 2024-05-06T21:13:45Z

Tests/SourceKitLSPTests/CodeActionTests.swift

+      """
+         1️⃣{
+         "name": "Produce",
+         "shelves": [


I think right now it’s only possible to invoke the refactoring from the opening {. I think it would be a lot more discoverable if you could invoke it from anywhere within the JSON.

No, the refactoring can be invoked from elsewhere in the closure. The test harness doesn't make it easy to check for places other than the start, though.

Hmm, I see what's going on. The parser isn't treating the whole blob as a single closure, so we sometimes end up in the "unexpected" area. I don't have a great fix at the moment.

Could you file an issue for that?

Sources/SourceKitLSP/Swift/CodeActions/ConvertJSONToCodableStruct.swift

ahoppen · 2024-05-06T21:17:38Z

Sources/SourceKitLSP/Swift/CodeActions/ConvertJSONToCodableStruct.swift

+}
+
+extension ConvertJSONToCodableStruct {


This is no longer a protocol-conforming extension 😜 Also, I’m not generally a fan of putting protocol conformances in extensions. I think it makes it harder to reason about which protocols a type conforms to.

Sources/SourceKitLSP/Swift/CodeActions/ConvertJSONToCodableStruct.swift

ahoppen · 2024-05-06T21:28:40Z

Sources/SourceKitLSP/Swift/CodeActions/ConvertJSONToCodableStruct.swift

+  init(value: Any) {
+    switch value {
+    case let string as String:
+      if string == "true" || string == "false" {


Interesting that JSONSerialization doesn’t deserialize those to an NSNumber. That does make me wonder: Does JSONSerialization deserialize "null" as NSNull or also as a string with value null? I don’t think we’ve got a test for that.

It leaves them all as strings. A "null" is a dreadful thing because it messes with the merging logic.

Is there any case that does produce an NSNull then?

ahoppen · 2024-05-06T21:29:40Z

Sources/SourceKitLSP/Swift/CodeActions/ConvertJSONToCodableStruct.swift

+      // Print any nested types.
+      for (typeName, object) in nestedTypes {
+        MemberBlockItemSyntax(
+          leadingTrivia: (typeName == nestedTypes.first?.0) ? .newlines(2) : .newline,
+          decl: object.asDeclSyntax(name: typeName)
+        )
+      }


I know this is a subjective opinion, but I would always put the nested types before the members.

Sources/SourceKitLSP/Swift/CodeActions/ConvertJSONToCodableStruct.swift

DougGregor · 2024-05-07T00:58:12Z

@swift-ci please test

DougGregor · 2024-05-07T00:58:23Z

@swift-ci please test Windows

DougGregor requested review from benlangmuir and ahoppen as code owners April 26, 2024 00:05

ahoppen reviewed Apr 26, 2024

View reviewed changes

DougGregor force-pushed the convert-json-to-codable branch from f1566e5 to e54c99e Compare May 6, 2024 20:58

Handle unexpected nodes following a closure

037e55e

ahoppen reviewed May 6, 2024

View reviewed changes

Address review comments

d4bbf9c

DougGregor enabled auto-merge May 7, 2024 00:58

DougGregor merged commit 7e5f87f into swiftlang:main May 7, 2024
3 checks passed

DougGregor deleted the convert-json-to-codable branch May 7, 2024 04:01

DougGregor mentioned this pull request May 7, 2024

[6.0] Syntactic refactorings "Add documentation" and "Add Codable structs for JSON" #1235

Closed

ahoppen mentioned this pull request May 7, 2024

Merge main into release/6.0 #1236

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a syntactic "Add Codable structs from JSON" code action #1205

Add a syntactic "Add Codable structs from JSON" code action #1205

DougGregor commented Apr 26, 2024 •

edited

Loading

DougGregor commented Apr 26, 2024

DougGregor commented Apr 26, 2024

DougGregor commented Apr 26, 2024

DougGregor commented Apr 26, 2024

DougGregor commented Apr 26, 2024

DougGregor commented Apr 26, 2024

DougGregor commented Apr 26, 2024

DougGregor commented Apr 26, 2024

ahoppen left a comment

ahoppen Apr 26, 2024

DougGregor May 6, 2024

ahoppen May 6, 2024

ahoppen Apr 26, 2024

ahoppen Apr 26, 2024

ahoppen Apr 26, 2024

ahoppen Apr 26, 2024

DougGregor commented May 6, 2024

DougGregor commented May 6, 2024

DougGregor commented May 6, 2024

DougGregor commented May 6, 2024

DougGregor commented May 6, 2024

ahoppen left a comment

ahoppen May 6, 2024

DougGregor May 7, 2024

DougGregor May 7, 2024

ahoppen May 7, 2024

ahoppen May 6, 2024

ahoppen May 6, 2024

DougGregor May 7, 2024

ahoppen May 7, 2024

ahoppen May 6, 2024

DougGregor commented May 7, 2024

DougGregor commented May 7, 2024

Add a syntactic "Add Codable structs from JSON" code action #1205

Add a syntactic "Add Codable structs from JSON" code action #1205

Conversation

DougGregor commented Apr 26, 2024 • edited Loading

DougGregor commented Apr 26, 2024

DougGregor commented Apr 26, 2024

DougGregor commented Apr 26, 2024

DougGregor commented Apr 26, 2024

DougGregor commented Apr 26, 2024

DougGregor commented Apr 26, 2024

DougGregor commented Apr 26, 2024

DougGregor commented Apr 26, 2024

ahoppen 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

DougGregor commented May 6, 2024

DougGregor commented May 6, 2024

DougGregor commented May 6, 2024

DougGregor commented May 6, 2024

DougGregor commented May 6, 2024

ahoppen 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

DougGregor commented May 7, 2024

DougGregor commented May 7, 2024

DougGregor commented Apr 26, 2024 •

edited

Loading