Hey 👋 I'm Eve, the friendly bot watching over SwiftGen 🤖

Thanks a lot for your contribution!

Seems like everything is in order 👍 You did a good job here! 🤝

Generated by 🚫 Danger

Tagging this 6.2.1 for now, unless it's done before we create the 6.2.0 PR.

Finally, split the YAML-Demo page in the demo Playground in 2 separate pages, extracting the JSON example in its own page

I didn't quite understand the playground. Is that embedded code generated by SwiftGen automatically?

Currently we copy code from one of the generated files (for the output tests), and sometimes process them a bit as needed (to keep the pages as minimal as possible)

Currently we copy code from one of the generated files (for the output tests), and sometimes process them a bit as needed (to keep the pages as minimal as possible)

hmm, do we need to make any changes here then? since the generated code will be the same, I don't see why we need to update the playgrounds

If the test changes that @AliSoftware mentioned are implemented, then that'll mean that the resources and generated code will change (and we'll have a separate JSON page instead of 1 for both JSON & YAML). We'll get to that when we've covered the test changes.

If the test changes that @AliSoftware mentioned are implemented, then that'll mean that the resources and generated code will change (and we'll have a separate JSON page instead of 1 for both JSON & YAML). We'll get to that when we've covered the test changes.

i've updated the tests, but the thing is that the template tests still are using ymls. I don't think this is too much of a deal right now, so I wonder if we can do this clean up later (as we update the templates to use document, for example)

Ok, for the playground that's my bad, I missed that we already had a JSON-Demo page it seems. So no new page to create there and probably nothing much to change.

I think I confused this with the case of some playground pages in which we do have multiple examples in a single playground page (for example the Plist-Demo page… hell even the JSON-Demo page… where we have 2 examples in these pages, one to show the code for inline-swift4 templates and the other for runtime-swift4. For some reason that memory of this made me think for a minute that we had a Yaml-Demo page with 2 examples too, one for Yaml one for JSON… but that's not the case (and wouldn't make sense since indeed the outputs are the same for both anyway), and we already have the Json and Yaml demos separate in playground

The only thing you could include in that PR regarding playground changes is to update the examples – for example the Yaml-Demo page contains internal enum Configuration in its examples while after your change the generated code in tests (and the file in Fixtures/Generated/JSON/…) doesn't contain that anymore (since that file has move to JSON fixtures and is not used in Yaml tests. Technically, the playground page is not incorrect, but it could be confusing to someone looking both at the test generated fixtures and the playground 🤷 not a bit deal though

Looks like the tests are failing because the template is not stable anymore, the order of the keys is changing between runs:

 internal enum JSONFiles {
-  internal static let items: [[String: Any]] = [[AnyHashable("id"): "1", AnyHashable("name"): "Anna"], [AnyHashable("name"): "Bob", AnyHashable("id"): "2"]]
+  internal static let items: [[String: Any]] = [[AnyHashable("name"): "Anna", AnyHashable("id"): "1"], [AnyHashable("name"): "Bob", AnyHashable("id"): "2"]]
 }

This is not surprising given that Dictionary doesn't have stable keys. I wonder why this wasn't a problem before (and if there's an easy way to fix it).

Ah, this has indeed been a problem in other places in the past.

This is why we order most of the content alphabetically in most of the places. Like the order of the entries in .strings is sorted before being output (see StringsContext.swift)

We might need to do the same for the JSON content then. Indeed not sure why this was not a problem before.

Maybe it was sheer luck that the order on macOS (our dev machines) and Linux (CI) were the same in the past implementation. Maybe it was due to some implementation details of Yams when parsing the file.

In any case the fact is that JSONSerialization is not deterministic in the keys orders (as expected for a dictionary) so we'll likely need to add some sorting indeed when building the context. The bad news is that we'll need to do that sorting on embedded dicts recursively too, to ensure consistency at all levels of the JSON structure being parsed… (we might want to add just another fixture that contains some deeper JSON structure to test that part btw)

PS: in case you were thinking of using the special build setting that we can use in Xcode to make Dictionary hashes be stable across runs for testing purposes, this is not really a solution since we also need to generate stable outputs in production, not just during tests, so that the generated code is also stable when the user runs swiftgen on every build via a script build phase

Note on that "sorting of keys" issue:
This was actually adressed in Stencil itself (stencilproject/Stencil#242), it'll sort dictionaries before iterating if it detects a [String: Any].

What has changed that defeats that check? And is there something that can be adressed in Stencil? I'm preparing a release there, so we can include a fix if needed.

Note on that "sorting of keys" issue:
This was actually adressed in Stencil itself (stencilproject/Stencil#242), it'll sort dictionaries before iterating if it detects a [String: Any].

What has changed that defeats that check? And is there something that can be adressed in Stencil? I'm preparing a release there, so we can include a fix if needed.

so I think the issue is not the order in the template, but the order inside a template variable

The template outputs variables using a for loop:
https://github.com/SwiftGen/SwiftGen/blob/develop/templates/json/inline-swift5.stencil#L63

idk what's up with GitHub this evening. Can't post a PR review nor comment inline either.

Documentation/SwiftGenKit Contexts/Json.md, line 5

typo: s/mutiple/multiple/ + wording s/each file/each file's content/

-The JSON parser accepts mutiple files or directories (which it'll recursively search). Each file will be loaded into the context, but the parser will also generate metadata about the structure of the file.
+The JSON parser accepts multiple files or directories (which it'll recursively search). Each file's content will be loaded into the context; but the parser will also generate metadata about the structure of the file content.

This diff actually needs to be applied to Documentation/SwiftGenKit Contexts/Yaml.md as well

Documentation/SwiftGenKit Contexts/Json.md, line 38

I know @djbe said in an earlier feedback that we don't document deprecated keys in contexts, but then it looks strange to have that documents: key in the example while it not being documented in the previous section…

imho we should either document that documents: key and mention it's deprecated, or omit it from the example (even if that makes the example not completely true to what is actually generated because of retro-compatibility until SwiftGen 7) so that it's less confusing.

Tests/TemplatesTests/JsonTests.swift

nit: It seems like Contexts.all only refers to ["empty", "all"] and doesn't reference configuration and array contexts. That might be fine (maybe we don't want to test all the cases everywhere and pollute the tests too much) but then we might consider renaming that variable to something other than all? commonTestSet or something?

btw wondering if we should merge this in develop as soon as we officially approve it, or if we should wait to have a fix for #712 before we merge that transition to JSONSerialization. I know the issue with #712 was already present before that PR though, but wondering if that already-existing issue will start to become way more common and thus more problematic if we merge that migration to JSONSerialization 🤔

There are issues with the inline templates in general:

• unstable output Unstable JSON/YML inline templates #712
• strings issues (multiline, quotes, ...) JSON: Inline template strips newlines from string values #551

Seeing as those are already present in the current & older releases, I don't think this PR should be held back because of them.

Ok, updated my previous comment with my latest feedback. Mainly just a typo and a question about including documents key or not in docs example

I'm merging everything and processing the feedback (+ fixing the typos in other places).

Note: @AliSoftware we already use the all context in other parsers, if we rename it here, we should rename it everywhere. And TBH we can delegate that to another PR.

I'm merging everything and processing the feedback (+ fixing the typos in other places).

👍 cool. I see that you fixed s/mutiple/multiple/ in those other places, but did not apply the same s/each file/each file's content/ fix everywhere tho 😅

Note: @AliSoftware we already use the all context in other parsers, if we rename it here, we should rename it everywhere. And TBH we can delegate that to another PR.

Ah, right. Let's keep Context.all then 👍

just another nit: I've also noticed that we seem to use inconsistent case in fine names for Documentation/SwiftGenKit Parsers vs Documentation/Parsers (Json.md vs json.md for example). (sorry, I'm quite nitpicky lately, that's just my perfectionist side speaking… hope you don't take it the wrong way 😬)