Swiftgen: [Internal] Reorganize Frameworks to extract the Stencil nodes ("The Great Split")

Created on 13 Dec 2016  Â·  21Comments  Â·  Source: SwiftGen/SwiftGen

There is multiple aims in this issue we want to discuss:

Organization

We'll be reorganising the GenumKit.framework to split it into:

  • StencilSwiftKit.framework, containing only the Stencil nodes specific to SwiftGen/Sourcery and don't have their place in Stencil proper
  • SwiftGenFramework.framework for the rest of the code currently in GenumKit responsible for parsing the various input files and generating the contexts accordingly

We'll probably move each of those two frameworks in their own repos and define dependencies properly, so that both SwiftGen and Sourcery can depend on StencilSwiftKit and that each of them have their own podspec and versionning life cycle.

Cleaning up the Stencil Nodes & Filters in GenumKit

  • [ ] What are the ones that should now be integrated into @kylef 's Stencil proper directly?
  • [ ] What are the ones that are no longer needed since Stencil evolved (I'm looking at you, JoinNode)
  • [ ] What are the ones that should be moved into a future StencilSwiftKit.framework, so that wet can share them with @krzysztofzablocki 's Sourcery?

See associated GitHub Project here: "The Great Split"

internal

Most helpful comment

Finally had some little time to take a look at those, sorry it took so long.

  • PRs in templates repo reviewed. Need a bit of changes but should be really quick fixes.
  • PR in StencilSwiftKit merged.
  • PR in SwiftGenKit needs to use whitelist instead blacklist, otherwise good

@djbe I'll let you adjust your PRs.
I've officially migrated AliSoftware/SwiftGen to SwiftGen/SwiftGen and will take a look at updating the Rakefile so we can release 4.2 using Rake even with this new reorganised setup.

All 21 comments

What should we move into Stencil:

  • all filters, except escapeReservedKeywords
  • SetNode
  • MapNode

You mean move to Stencil itself, not to StencilSwiftKit… right?

Also, for the same reasons as for escapeReservedKeywords, I don't think the swiftIdentifier filter should be part of Stencil proper

I suggest we (mis)use a project board for this:
The Great Split 😆

First add blocks for each item to the first column, except those related to pure command line stuff. Then we can move one by one each to the correct column, discuss if needed and also the impact of moving it, etc...

:+1: great idea

Not sure if we should treat that in the same issue or not, but I think we should also review the Stencil Contexts we expose in GenumKit / future SwiftGenKit, because now that Stencil offers more features, like "Filters with Parameters" and stuff like that, and now that we can implement the MapNode, a lot of variables we exposed in the Stencil contexts "just because it wasn't possible to construct that content in the template directly" will disappear.

A typical example is with the Strings context, where we exposed p0: Int, p1: String as the string.params.typednames variable in the context because there was no way to iterate two arrays (param names & param types) at the same time (zip-style) nor join them properly with a custom separator, but now with the latest Stencil and nodes we could (for example using a MapNode to transform [Int, String] into [p0: Int, p1: String] then use join(", ")), so the string.params.typednames context variable isn't needed anymore (and in fact isn't flexible enough to adapt to the Swift 3 naming conventions)

@AliSoftware While I think the development will be in parallel, we'd best keep that part of the discussion in #231

Oh right, indeed. Forgot we had a dedicated issue for that. So many pending stuff ^^

I wonder if percent and int255toFloat are still needed if Stencil implements arithmetic operator (I'm not sure Stencil has +/-/*// yet, but I think it would be a great addition to Stencil if it doesn't have it already)

@krzysztofzablocki @djbe : I've created both https://github.com/AliSoftware/SwiftGenKit & https://github.com/AliSoftware/StencilSwiftKit repositories (totally empty for now) and added both of you as collaborators on them so that we can start the Great Split whenever we're ready (which should be pretty soon (especially since #233 seems to pass tests again, and might thus probably be merged soon)

Woo 🎉

Alright, as discussed with @djbe in Slack, I finally removed AliSoftware/SwiftGenKit and AliSoftware/StncilSwiftKit, and instead created a dedicated SwiftGen GitHub org that will host:

  • SwiftGen/swiftgen : the code for the CLI wrapper. That will in fact be this very AliSoftware/SwiftGen repo moved/transfered to SwiftGen/swiftgen so that we keep the history, stargazers and watchers, etc
  • SwiftGen/SwiftGenKit: the framework handling all the parsing of the resources to turn them into Stencil contexts
  • SwiftGen/StencilSwiftKit: the framework containing the specific Stencil nodes useful for Swift code generation. Will be a dependency of both SwiftGen/SwiftGenKit and Sourcery
  • SwiftGen/templates: will contain the templates as well as their fixtures needed to unit-test them

_Note: SwiftGen/templates will probably be also set up as a git submodule for some of the other repos when they'll be needed to write unit tests for those repos._

Templates versionning

💡 To be sure templates are tagged with the version of SwiftGenKit they're compatible against, we should later design some comment at the top of the templates to indicate which major version of SwiftGenKit each template is supposed to work with, like {# SwiftGenKit Version: 4 #} or similar.

If SwiftGenKit detects such a line at the top of a template and see that the version doesn't match its major version, it should at least print a warning when using such a template.

Just added a GitHub PR template to warn potential new contributors that the project is being frozen for the Great Split so that might not be the best time for a PR.

  • [ ] We should remember to remove that once the Great Split is finished.

I think most of the stuff is ready on the other repos once we merge the (non-breaking) PRs. If both of you could do a quick review of them, and then @AliSoftware merge then (once Travis unsticks itself).

Todo:

  • [x] Merge the PRs in the templates repo
  • [x] Merge the PRs in the 2 Kit repos
  • [x] Update the Kit submodules to point to the latest templates repo
  • [x] Create 1.0.0 release for SwiftGenKit and push it to CocoaPods
  • [x] Create 1.0.0 release for StencilSwiftKit and push it to CocoaPods (waiting for a new release of Stencil)
  • [x] Update the great-split branch to use the released versions of both Kits and the latest templates version
  • [x] Remove the PULL_REQUEST_TEMPLATE.md text from .github/
  • [x] Release SwiftGen 4.2 (or 5)
  • [ ] ...
  • [ ] profit!

Finally had some little time to take a look at those, sorry it took so long.

  • PRs in templates repo reviewed. Need a bit of changes but should be really quick fixes.
  • PR in StencilSwiftKit merged.
  • PR in SwiftGenKit needs to use whitelist instead blacklist, otherwise good

@djbe I'll let you adjust your PRs.
I've officially migrated AliSoftware/SwiftGen to SwiftGen/SwiftGen and will take a look at updating the Rakefile so we can release 4.2 using Rake even with this new reorganised setup.

Just waiting on:

  • [x] SwiftGen/StencilSwiftKit#19 to finish building on CI
  • [x] so we can then use it and merge SwiftGen/templates#19
  • [X] And maybe also merge SwiftGen/templates#17 while we're at it

Then I'll finally be able to: update the submodules to point at latest templates, create tags for Kit frameworks & push to CP, update the great-split branch to point to those releases, merge the great-split branch and tag it 4.2!

If you want, you can also merge https://github.com/SwiftGen/StencilSwiftKit/pull/11 (the map node PR).
None of the templates use and/or need it now, but some users might want it + could be handy for new PRs such as the code gen one.

Ah and before releasing 4.2 we should at least call enrich(context: ) to inject the env var in the context — even if we don't implement #253 with --param just yet because we should stop postponing that 4.2, at least we would have the env auto-injected and be able to get rid of that annoying warning for storyboards.

I've already added that code in the great split branch ^^ (including the param support)

@djbe I've started to recreate milestones and label PRs with 4.2.0 in this repo ; I think it's already done for versions 1.0 in the other repos?

It's done in both kit repos and the templates repo

Was this page helpful?
0 / 5 - 0 ratings

Related issues

taykay08 picture taykay08  Â·  3Comments

kacper1703 picture kacper1703  Â·  3Comments

djbe picture djbe  Â·  6Comments

valerianb picture valerianb  Â·  7Comments

drjasonharrison picture drjasonharrison  Â·  5Comments