-
Notifications
You must be signed in to change notification settings - Fork 105
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 #175
Conversation
Note that this introduces a slight breaking change if users have been overriding the |
- UnitTests: | ||
- Resources: | ||
- UnitTests-Info.plist | ||
- Helpers: |
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.
how come have this for swift but not objc?
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 want to do another pass to standardize the Obj-C structure based on this newer structure. That's out of scope for this PR though.
@thoughtbot/ios Any feedback on this? Especially looking for feedback on the UI and directory structure. |
👍 on this directory structure. |
@@ -30,6 +30,14 @@ def global_options | |||
exit | |||
end | |||
|
|||
opts.on('--[no-]swift', 'Set the language to Swift or Objective-C') do |swift| |
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 swift is the default and using --no-swift
forces it to use objective c? That seems just a little weird because it's not really an option but a binary choice that is fundamental.
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.
Should add that I don't know what the ideal interface for this would be.
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.
Maybe the opposite should be --objc
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 didn't want to do --objc
because that introduces a weird state where you could theoretically pass --swift
and --objc
at the same time, and the outcome would be totally determined by the order that you pass the flags. Given that it actually is a binary choice (we don't support other languages), using a boolean flag made more sense to me.
That being said, I was thinking about this on my way home yesterday, and there's a chance we could make the entire thing more flexible by using top level dictionaries for the language name, and then using a --language=
flag on the command line to choose. This could also be used to create non-language specific project templates. For example, given this structure:
app_target_groups:
empty:
- <%= project_name %>:
you could theoretically pass --template=empty
to the executable, and have liftoff use the proper structure. That would change the entire concept of language
to template
, but that's probably closer to what we're doing anyway.
The issue here would be that (when implemented the simplest way possible) overriding the app_target_groups
key would mean having to reproduce all of the other templates. Ideally, we would use some sort of Programming Magic to make that key (and the unit test target groups) stackable. So if you added an empty
template to the app target, you'd still get our objc
and swift
templates for free.
I think I've totally talked myself into this idea. Good job, me.
I just pushed a WIP update (it works, just doesn't have all of the features) that changes the format used to be more generic, and hopefully more flexible. |
ccd8a0c
to
fdf55d3
Compare
Ok, I think this is all set for final review. I'd also appreciate some eyes on the man pages (view in a gist here). Especially the |
then that template will be used for the test target. | ||
.Pp | ||
This setting can be overridden on the command line by using the | ||
.Ic --template Ar TEMPLATE_NAME |
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.
should this be single hyphen -template
?
👍 just confused on if there should be a single or double hyphen in front of |
It should be a double hyphen. If you look at the rendered version (above), you'll see that it's consistent. |
This is actually how Xcode names these files now. Might as well keep in sync.
This is needed to be able to run Swift code
This is accomplished by moving the directory structure keys under template names inside larger `app_target_templates` and `test_target_templates` keys. By setting the `project_template` setting in liftoffrc, the user can chose which template to use by default. You can also pass in the name of a template on the commandline with the `--template [TEMPLATE NAME]` flag.
config[key] = evaluated_templates(key) | ||
end | ||
|
||
config |
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.
How about Hash#merge
instead? Maybe something like:
def evaluated_configuration
initial_configuration.merge(template_groups_configuration)
end
def initial_configuration
default_configuration
.merge(user_configuration)
.merge(local_configuration)
.merge(@options)
end
def template_groups_configuration
TEMPLATE_GROUPS.inject({}) do |acc, key|
acc.merge(key => evaluated_templates(key))
end
end
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.
Oh, or just add another #merge
to the end of #evaluated_configuration
. Either way: #template_groups_configuration
could be broken out.
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.
Oh, yeah, good call. I like that a lot.
This lets users override parts of the target template keys, instead of having to override the whole thing. For example, this lets users only override the application target groups for the swift language. It also lets users add their own templates to their configs so that they can define their own custom templates
If the user provides an invalid template name, we don't want to continue. Instead of proceeding and leaving artifacts, we should fail early.
This adds support for generating Swift projects.
Yeah, I know, finally.