-
Notifications
You must be signed in to change notification settings - Fork 728
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adding code gen #188
Adding code gen #188
Conversation
Edited the previous comment with the new changes. I think this is ready for review now and merge once I've worked out the problems listed above. |
FYI, I also tested this on https://github.com/bwhiteley/JSONShootout for performance and this solution was faster than any solution there. I'll create a pull request for the repo once I've merged in these changes. |
I'm not sure it makes sense to see that new feature as a command specifically to generate models, rather than seeing it as a generic JSON input parser? And make the command Haven't had time to review all the code, but I'm not sure to see why your code would only be tied to model generation and not arbitrary content generation? |
So, this code takes JSON as an input and produces models. Naming it JSON seems weird, because that's the input not the output. It seems like all the other commands are based on the output. As in:
Will produce an enum of images. Possibly, you're saying this should be able to create anything? Which is sort of true I suppose...but by default, it produces models (if you don't specify a template), so seems more user friendly to name it model? |
Update on original issues and @AliSoftware's comment:
I think it's ready for review and merge now :D |
GenumKit/Stencil/Filters.swift
Outdated
|
||
return strings.joinWithSeparator(", ") | ||
} | ||
|
||
static func joinAnd(value: Any?) throws -> Any? { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It has been a while since I checked how Stencil evolved and what new features were brought by more recent versions, but I remember that @kylef wanted to implement filters with parameters. In case that already landed we could use that instead of defining a dedicated filter for each joiner string we need!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this is unfortunate. However, I can't see anything in the docs or code which suggests parameters are no supported:
https://github.com/kylef/Stencil/blob/master/Sources/Namespace.swift
open func registerFilter(_ name: String, filter: @escaping Filter) {
:/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've implemented this in master now, you can find an example of implementing a filter with arguments at stencilproject/Stencil@dfd57e9.
It's going to be a few days until I cut a release with this though, there are other things I want to get into this release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yay 🎉
@plivesey it's up to you if you prefer to depend on Stencil's master (we've done that before to wait for other dependencies to do a release) or wait a little until it gets official 😉
(Gonna review that once you've adapted this PR accordingly)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kylef sadly, I can't upgrade stencil because SwiftGen inherits some classes to modify some behavior. Since things aren't open, this won't compile.
Take a look at https://github.com/AliSoftware/SwiftGen/blob/master/GenumKit/Stencil/GenumTemplate.swift
Template and Namespace are both subclassed here. The code seems reasonable to me (so wondering if you'd consider making these open), but wondering if you have other ideas.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sub-classing template is still possible. I don't see any real reason you would need to subclass namespace, composition > inheritance. The use here could be replaced by a function creating a namespace. Tbh, I think I'll be rehauling part of the extension API in the next version to make it easier to create "components" containing multiple filters/tags.
I've updated to Stencil 0.7.1 in #219.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, and great. I'll apply your PR to mine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, not sure why I didn't catch this locally but there is a problem with my PR. I'll try fix it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the problem that Template isn't actually open
? Because I'm confused. For me on 0.7.1:
public class Template: ExpressibleByStringLiteral
https://github.com/kylef/Stencil/blob/master/Sources/Template.swift
GenumKit/Stencil/Filters.swift
Outdated
@@ -106,3 +126,83 @@ struct NumFilters { | |||
return "\(percent)%" | |||
} | |||
} | |||
|
|||
struct ModelParsingFilters { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aren't all those too specific? Not sure why we can't implement that in the template instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, this took me the longest time to work out. AFAIK, it is impossible to implement them in the template because of the limitations of Stencil. To take them one by one:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
parameterList
This one needs to create a list of params like: x: String, y: Int
The main problem here is that there is no trailing ,
. This means it's impossible to do in a template alone. So, I decided to use join
! But join
takes an array as a parameter, so I need to convert my input to an array. This is impossible to do in stencil. Basically, the main functionality which stencil seems to be missing is a map filter. Sadly, you can't just implement a map
componenet, because component's are rendered. You need a map
filter. But, since filters can't take parameters, you need to implement seperate filters for each map
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
equalityList
. Basically, the same problem as above. Need to create an array of ==
statements, and there is no map
function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
initializerList
yep same thing here too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
parameterType
This is needed by parameterList
and since it occurs so much in the code gen, I decided to make it a filter too. I could remove it as a filter, but I'd still need it as a private function, and doing this means I don't need to reimplement all this code in the template and makes it much easier to read.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main concern here is that this will grow as people add their own templates. This is unfortunate, but sadly, Stencil just really doesn't support lists well because the lack of a map
(oh, how I love map
). As people more methods details, we should be able to change these up to be as generic as we need. I tried to keep stuff as generic as possible creating lists for joining and not just outputting strings.
swiftgen-cli/base.swift
Outdated
import GenumKit | ||
import Stencil | ||
|
||
let baseCommand = command( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the need for that command?
swiftgen templates cat json-base
already covers outputting the verbatim content of a template…
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah. I didn't realize that. Perfect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pushed
@AliSoftware I've updated to use a filter with an argument instead of join&&. There is one outstanding issue: I had to manually edit the stencil pod to make template open. Waiting on @kylef to see what to do about that. |
Ok, cool, thx! I'm adding the WIP label to wait for #219 to be merged and Stencil to be updated in SwiftGen, so we could then rebase this PR on top of the changes (that would make your PR cleaner and easier to review if it doesn't contain the Stencil pod update along the rest of the changes) and review it then 😉 |
I'll make a release (0.7.2) which makes Template open soon and update my PR. |
GenumKit/Stencil/GenumTemplate.swift
Outdated
namespace.registerFilter("int255toFloat", filter: NumFilters.int255toFloat) | ||
namespace.registerFilter("percent", filter: NumFilters.percent) | ||
namespace.registerFilter("escapeReservedKeywords", filter: StringFilters.escapeReservedKeywords) | ||
namespace.registerTag("ifstrequal", parser: StrEqualsNode.parse) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this tag or the one below would be necessary with Stencil 0.7, you can use the new if
statement that takes operators [docs]. For example:
{% if something == "example" %}
{% endif %}
{% if something != "example" %}
{% endif %}
Got a basic model that almost compiles working
Updating the codgen to fix the comma issue and simplify some things Adding support for more things like Double and optional array
Adding marks to generated code Improving performance by using NSDictionary instead of [String: Any] A bunch of other work
Docs Adding base type Remaining model stuff to JSON Removing added cocoa pods stuff
Removing references to this in the docs Changing the static code generation in the docs to use `swiftgen templates cat model-base > /path/to/output/file`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've pushed a small merge with master.
My main issue with this PR is the 2-fase command, more specifically the static template that'll be embedded in SwiftGen. That's just my opinion though, but I'd like to discuss this 😄
The Readme entry could use a bit more work, I've made some small suggestions. Multiple paths is also something worth thinking about now, before we merge and release this (as it'll change the template syntax).
I haven't reviewed the actual template code yet, I'll do that later this weekend.
@@ -12,6 +12,8 @@ SwiftGen is a suite of tools written in Swift to auto-generate Swift code (or an | |||
* [`enums` for your NSStoryboards and their Scenes](#nsstoryboard) | |||
* [`enums` for your Colors](#uicolor-and-nscolor). | |||
* [`enums` for your Fonts](#uifont-and-nsfont). | |||
* [`classes` for your models](#models) | |||
* [`any code` for your JSON specs](#json) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole any code
section seems extraneous to me. I think there should be one section "Parse your json
files", and then one subsection in that about parsing models.
@@ -614,8 +616,196 @@ let sameFont = UIFont(font: FontFamily.Helvetica.Bold, size: 16.0) // iOS | |||
let sameFont = NSFont(font: FontFamily.Helvetica.Bold, size: 16.0) // OS X | |||
``` | |||
|
|||
--- | |||
## Models | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment above. I'd rename this to JSON
, and a general description about how a user could parse whatever they want using custom templates. Explain that there's a spec
variable.
Then add a subsection about model-parsing.
```sh | ||
swiftgen json /path/to/model/file -t model | ||
swiftgen templates cat model-base > /path/to/output/file | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not really a fan of having a command that's effectively split into 2 dependent parts. I understand you want to reduce code duplication, and I wholeheartedly agree with that sentiment.
That said, would it be better to move the contents of model-base
to a gist or mini-library? There are new swift JSON parsing libraries appearing every minute, and everyone has their preferences about which one to use. I'm just wondering if it's worth adding a static template to SwiftGen just for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's actually how I originally had it but @AliSoftware asked me to change it to this model. I think he didn't want to maintain another cocoapods library? We'd also have several of these if people added their own codegen.
I agree that there are a bunch of JSON parsing libraries appearing, but most are optimized for making the code look pretty instead of actually providing useful functionality. Since we code gen things, being super terse isn't necessary and we can instead provide useful features.
I'd be fine to revert back to doing this as a library though - I don't have particularly strong feelings about it. @AliSoftware what do you think ^^^?
swiftgen templates cat model-base > /path/to/output/file | ||
``` | ||
|
||
This takes a JSON spec and converts it into a model file which supports parsing from a dictionary, converting back into a dictionary, equality, mapping, and three different initialization options. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make bullet points about what's supported.
|
||
The input is a JSON file which creates a spec for the object. The schema is a custom schema but is based off a few standards (AVRO, Pegasus, JSON Schema). The spec takes a type name and a list of fields. Each field specifies it's type, name, whether it's optional and a doc. It can also specify `array` and `element` for an array of elements. The type and element field can be anything. The model generator simply converts it to an uppercase string for the type. | ||
|
||
The model template also requires some static code which makes the generated code smaller. This static code is outputed by `swiftgen templates cat model-base > /path/to/output/file`. You should generate this code as well as the model code for each of your JSON specs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above comment, this could simply become "Add the line pod 'MyMiniParsingLibrary'
to your Podfile or library of choice".
``` | ||
</details> | ||
|
||
## JSON spec |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See first comments above, I'd rearrange this.
|
||
let expected = Fixtures.string(for: "Models-Dir-Basic.swift.out") | ||
XCTDiffStrings(result, expected) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add more tests:
- empty input model
- incorrect input model
let parser = JSONFileParser() | ||
|
||
do { | ||
try parser.parseFile(path: path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be interesting to rewrite your parser to accept multiple input files, see #213 for more information. If you do, don't forget to add unit tests for those.
@@ -0,0 +1,120 @@ | |||
// Generated using SwiftGen, by O.Halligon — https://github.com/AliSoftware/SwiftGen | |||
|
|||
public final class {{ spec.type }}: DataModel, Equatable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment above, would be cool to accept multiple spec
s.
Hi @plivesey, with the latest SwiftGen release we've split the whole project into many sub-parts. If you'd like I can help you get started on reworking this to match the project changes. The best place to start is with the parser, which'd go in the SwiftGenKit repository. It might be best to open an issue there first though, so we can discuss the scope of this feature, and what would fit in the SwiftGen project and what wouldn't. |
@AliSoftware @djbe This was a need that I also had. So as I use RocketData that was created by @plivesey and SwiftGen I arrived here. I decided to create a project and that satisfy this need, the biggest motivation to create a separated project is to support more languages like Kotlin and Java. I would love to hear what do you think. This is also somehow related to SwiftGen/SwiftGenKit#30 from @Liquidsoul |
@hebertialmeida Hey, thx for the feedback
I'm not sure I understand that rationale? as SwiftGen, despite its name, is capable of generating whatever you like, as it's based on templates. In fact, in a side projects I once used it to generate Java code using a custom template 😉 The next big pressing feature for SwiftGen after the long-awaited 5.0 release tomorrow will be support for Xcode 9 (Swift 4 templates + support for colors in xcassets). Once this lands, we'll probably start working on the support for swiftgen.yml config file, a JSON parser (which will address the current issue) and maybe merging some repos (in light of what splitting in 4 GH repos led to in term of maintenance and speed of introducing new features). So JSON parsing is definitely on the radar — and the solution will probably be way more simple to implement now, as it's simply gonna be a pass-thru class conforming to Parser protocol which will automatically leverage support for multiple input files, ability to use |
BTW I see that you already implemented the "use an yml file to configure the tool" feature in ModelGen (which is something we definitely want for SwiftGen, there's an issue pending about that somewhere), would love some contribution on that ;) |
Quickly looking at your SourceCode, and seing it uses SwiftGenKit already 👍 , I think we could indeed share some implementations there, like your |
I can definitely try to help with the yml file, I did that for ModelGen and it was simple. I don't have experience with installers, later I want to add it to Homebrew or something to make it easy. It is built on top of SwiftGenKit and Stencil with Commander already, I have based on SwiftGen and Sourcery 😄 |
Correct me if I'm wrong, but you don't actually use SwiftGenKit, do you? You depend on it, sure, but I don't see you importing it anywhere. You do use StencilSwiftKit though 👍 |
@djbe OMG, Yeah you are right, sorry about that, I started using it but then I have created my parsers based on it, then I forgot to remove the dependency. 👍 |
I've mostly finished all the code for the initial commit. But need some advice/help/more work on a few things:
Input:
Output:
Usage: