Skip to content
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

Swift-ifed templates #368

Closed
wants to merge 3 commits into from
Closed

Conversation

radianttap
Copy link
Contributor

@radianttap radianttap commented Jun 18, 2017

Summary of Changes

This is extensive rewrite of the Swift templates. Highlights:

  • removed _ModelClass subclass, no need for it in Swift
  • there‘s only one ModelClass in human template + extension in machine template
  • proper support for Optional attributes with "Use Scalar Type"
  • replaced usage of NSSet with proper Set<ModelClass>
  • changed func entityName() to var entityName
  • removed add* and remove* methods since they are not needed when Set is present
  • replaced Attributes / Relationships enums with struct with static let String constants, to avoid use of .rawValue all over the code
  • moved those structs into the extension, so they can be properly "namespaced", as ModelClass.Attributes.xxx

Addresses

None. Simply an improvement over existing stuff.

@gskbyte
Copy link

gskbyte commented Jan 4, 2018

Hello! Are there any plans to integrate this?

@atomicbird
Copy link
Collaborator

I like these changes. Right now they break some of the assumptions in CI. I'm reluctant to merge a change that breaks CI but since I've never used Travis I'm not in a position to change the test to reflect the new generated code right now. Although the two-level class hierarchy isn't needed with Swift, I wonder if you'd consider an alternate version of this PR that made most of the same changes but kept the same class hierarchy?

@radianttap
Copy link
Contributor Author

I've been using these templates for so long now, I don't see moving back to multiple classes. I would not have time to use/test them properly in actual code.

@atomicbird
Copy link
Collaborator

OK that's fair. I'm trying to get a new release ready and I'd like to merge as many useful things as possible. I'm not sure how to handle this one yet-- it's good stuff that I'd like to include, but I'll need to investigate the project CI a bit first. Thanks for the PR.

@atomicbird
Copy link
Collaborator

For anyone who's interested, I'm working on getting this merged, and I'll modify the tests to make it pass. The problem with currently-failing tests is not changing from two classes to class+extension, but a couple of easily corrected errors.

However, the one thing I'm getting hung up on is the fact that this PR puts generated initializer code into the human-editable Swift file. I get why that was done, but at the same time I really think that generated code needs to be kept out of that file. I'm trying to see what I can do to resolve this, to get the improved Swiftiness while still keeping a clear division between generated and editable source files.

@atomicbird atomicbird mentioned this pull request Aug 4, 2018
@atomicbird
Copy link
Collaborator

Changes from this PR have been incorporated into #373. Thanks a ton, @radianttap!

@atomicbird atomicbird closed this Aug 4, 2018
@atomicbird
Copy link
Collaborator

Also, this PR added additionalHeaderFile back to the Swift template, which proved to be a compatibility problem. Commit aaea23b adds the new, related additionalImports, which should serve the same purpose for Swift.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants