Adding support to creating custom schemes #225

marcelofabri · 2015-05-25T02:34:18Z

This adds basic support to creation of custom schemes on .liftoffrc:

schemes:
    - name: <%= project_name %>-CI
      actions:
          test:
              build_configuration: Debug-CI
          launch:
              build_configuration: Debug-CI
          profile:
              build_configuration: Release-CI
          archive:
              build_configuration: Release-CI
          analyze:
              build_configuration: Release-CI

actions can be test, launch, profile, archive or analyze and the only supported customization so far is build_configuration, but this can be extended to other options (such as "Launch due to background fetch").

I've done specifically for build_configuration but maybe we should support automatically (i.e. without having references in liftoff) all supported options in a scheme.

As my other PR, this still needs tests and documentation.

I'm also not particularly happy with the chain of ifs in create_schemes, but I couldn't figure a better way in Ruby (geez, I miss Objective-C nil behavior).

gfontenot · 2015-06-11T23:58:56Z

Hey, sorry, I didn't forget about this. Promise. I'm going to look over these ASAP.

marcelofabri · 2015-06-12T03:00:34Z

No problem!

marcelofabri · 2015-07-27T02:14:39Z

@gfontenot Have you gotten a chance to look at this?

bitcrumb · 2015-08-10T18:53:43Z

Would be awesome of this could get integrated anytime soon! 👍🏻

jakecraige · 2015-08-10T19:30:24Z

Thanks for implementing this @marcelofabri ! I have a few things I'd like to see addressed if you're up for it.

First, Follow the thoughtbot Ruby style guide. I plan on enabling houndci.com soon once I have time to update the config, but until then we have to manually confirm we're following it.

A few violations I see here are:

Avoid conditional modifiers (lines that end with conditionals).
Prefer double quotes for strings.
Use a trailing comma after each item in a multi-line list, including the last item.

Second, that large nested if structure. I see you aren't too happy with it either, one thing I'd suggest to clean it up is to extract methods out. If you take the content of each if and put it in it's own method that would be a great start. Another idea would be for the arrays to do variable || [] which would prevent checks before iterating over it. After those is likely you may spot some other optimizations to do.

Lastly, Docs and tests. You said this still needs them, are you waiting for us to approve the implementation before doing those tasks?

What do you think?

marcelofabri · 2015-08-11T00:20:01Z

Thanks for the feedback, @jakecraige.

About the violations:

OK
This file already was using single quotes for strings. Is there a reason for using them where it's currently being used?
Where have you seen this? I couldn't find it on this PR.

About the ifs: will do.

About docs and tests: I'd like an OK on the feature itself before spending too much time on them. Also, is there a easy way to edit the man help file?

I also would like some help to start the tests. I haven't found any tests on project_builder.rb.

jakecraige · 2015-08-11T00:28:06Z

lib/liftoff/project_configuration.rb

@@ -20,7 +20,8 @@ class ProjectConfiguration
      :xcode_command,
      :extra_config,
      :extra_test_config,
-      :deployment_target
+      :deployment_target,
+      :schemes


This is where the trailing comma is needed

But isn't the rule only for lists? Is this any different than https://github.com/thoughtbot/guides/blob/master/style/ruby/sample.rb#L19? (I'm not a Ruby developer, so forgive me if I'm writing something dumb)

Yeah, what you linked to is a method definition. It's actually a syntax error there so we can't do it. The example that shows this case is here

Actually, I get an error if I try to add a comma:

liftoff/lib/liftoff/project_configuration.rb:26: syntax error, unexpected tSYMBEG, expecting keyword_do or '{' or '(' (SyntaxError) attr_writer :author, ^

Whoops! I didn't see this was an attr_accessor list here. You're correct!

jakecraige · 2015-08-11T00:40:22Z

For 2, it's something we'd like to change going forward so we can retro-actively change them over when we touch that part of the code. For this one, use double-quotes in your new code since you didn't alter any existing code that uses single ones. That make sense?

I'm not familiar with editing the man file actually, I'll defer to @gfontenot on that one to see if he has any tricks for that.

For the tests, You can look to the project_configuration_spec.rb for an example of how to set up the base spec.

From there I see 2 possibilities. the latter being my preferred approach:

Set up a test forProjectBuilder#create_project method and run / assert that it creates the custom schemes. Since we don't have any test infrastructure to set up actual xcode projects, you'll probably have to do a bit of stubbing to get it all working properly.
Extract a class like SchemeBuilder and move all your new code to there and test that. It'll be much easier to test that class in isolation than testing it through this ProjectBuilder.

What do you think?

marcelofabri · 2015-08-11T01:29:42Z

@jakecraige I think I covered all the style violations and have dealt with the nested ifs. Could you please take a look if something is missing?

About the tests, I like the 2nd approach. However, to fully testing, I'd still need a project for creating a scheme, wouldn't I?

jakecraige · 2015-08-11T05:48:07Z

My bad on the trailing comma stuff. I didn't notice that was an attr_reader and not an array.

It's looking much better now. Great work!

With the new class/testing, you'll probably need to stub out the Xcode project so that we don't need an actual one to work with. The new class would probably take the project as a param in the initializer do you could pass it a fake one and make assertions off of that.

jakecraige · 2015-08-11T05:49:45Z

lib/liftoff/project_builder.rb

+      xcode_project.generate_scheme
+
+      if @config.schemes
+        @config.schemes.each do |scheme_config|


What are your thoughts on making a method called schemes that returns this config or an empty array to remove this conditional?

gfontenot · 2015-08-11T11:37:12Z

Also, is there a easy way to edit the man help file?

The man pages can be found here: https://github.com/thoughtbot/liftoff/tree/master/man

Editing them shouldn't be too difficult, but it's an intimidating format. Try looking at some similar examples (like the extra_config setting) and see if you can figure it out. If it's confusing, ping me and I'll help out.

marcelofabri · 2015-08-11T12:40:50Z

@gfontenot: thanks!

@jakecraige: I've extracted the changes to another class. Could you please take a look? I'll try to get the tests and docs figured out later today.

jakecraige · 2015-08-11T16:28:50Z

lib/liftoff/scheme_builder.rb

@@ -0,0 +1,50 @@
+module Liftoff
+  class SchemeBuilder
+


Remove this extra newline?

jakecraige · 2015-08-11T16:37:56Z

Thanks for the great work!

borwahs · 2015-08-11T20:36:19Z

This feature looks awesome. Was thinking about working on this but glad someone beat me to it!

@marcelofabri - does this have the ability to define custom configuration (xcconfig) files with the custom schemes baked in?

marcelofabri · 2015-08-12T03:48:12Z

@borwahs currently, no. My idea was to use this with #224, so you could have something like this in your .liftoffrc:

build_configurations:
    - name: Debug-CI
      type: debug
    - name: Release-CI
      type: release

extra_config:
    Debug-CI:
        GCC_PREPROCESSOR_DEFINITIONS:
           - $(inherited)
           - APP_CI=1
    Release-CI:
        GCC_PREPROCESSOR_DEFINITIONS:
           - $(inherited)
           - APP_CI=1

schemes:
    - name: <%= project_name %>-CI
      actions:
          test:
              build_configuration: Debug-CI
          launch:
              build_configuration: Debug-CI
          profile:
              build_configuration: Release-CI
          archive:
              build_configuration: Release-CI
          analyze:
              build_configuration: Release-CI

borwahs · 2015-08-12T04:38:58Z

@marcelofabri - that looks awesome. I'll be looking forward to it when it gets merged in!

marcelofabri · 2015-08-28T00:18:19Z

@jakecraige Sorry for the delay, but I was able to play with this only today. I have done 2 tests around this, but I wasn't able to test an important part (the setup of buildConfigurations).

Do you have any ideas?

marcelofabri · 2015-08-29T06:32:19Z

I was able to create a test by using a plain XCScheme (instead of mocking it).

I also updated Xcodeproj so I could use test_action & friends, avoiding digging into the scheme XML.

Unless I missed something, I think this is ready to merge! 😄

jakecraige · 2015-08-29T07:47:20Z

Thanks! I'm traveling this weekend and beginning a new project next week so
I may not be able to take a look till late next week.
On Fri, Aug 28, 2015 at 11:32 PM Marcelo Fabri notifications@github.com
wrote:

I was able to create a test by using a plain XCScheme (instead of mocking
it).

I also updated Xcodeproj so I could use test_action & friends, avoiding
digging into the scheme XML.

Unless I missed something, I think this is ready to merge! [image:
😄]

—
Reply to this email directly or view it on GitHub
#225 (comment).

jakecraige · 2015-09-04T13:30:20Z

lib/liftoff/scheme_builder.rb

+    end
+
+    def create_schemes
+      @xcode_project.generate_default_scheme


What do you think about making @xcode_project and @config private attr_readers and using that throughout this class to access them?

We have an open PR for our guides that recommends this that looks like it's probably going to be merged. thoughtbot/guides#331

Should I keep using @xcode_project and @config Inside initialize?

marcelofabri · 2015-09-04T15:37:11Z

@jakecraige Thanks for taking a look at this! I've just pushed 6ad5994, which fixes most of the comments you've made.

jakecraige · 2015-09-04T20:05:21Z

man/liftoffrc.5

+(test, launch, profile, archive or analyze). In order to create additional
+schemes, you should add the
+.Ic schemes
+key in your


Is this missing a word after "your"?

No, because .Nm in the next line will be replaced by liftoffrc

jakecraige · 2015-09-04T20:22:07Z

Code wise, this is looking great and I think it's ready to go! Thanks for being patient and working through this with me @marcelofabri

Could you squash the commits down?

marcelofabri · 2015-09-04T23:21:44Z

@jakecraige Thanks for the help and all suggestions. It's definitely better now!

jakecraige · 2015-09-08T16:04:32Z

Merged in 4c99412 Thanks again.

marcelofabri mentioned this pull request May 25, 2015

Include a custom configuration/scheme pair #71

Closed

marcelofabri mentioned this pull request Jun 10, 2015

Adding support to creating custom build_configurations #224

Closed

jakecraige reviewed Aug 11, 2015
View reviewed changes

lib/liftoff/scheme_builder.rb

@@ -0,0 +1,50 @@

module Liftoff

class SchemeBuilder

Copy link

Contributor

jakecraige Aug 11, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this extra newline?

marcelofabri changed the title ~~[WIP] Adding support to creating custom schemes~~ Adding support to creating custom schemes Aug 29, 2015

jakecraige reviewed Sep 4, 2015
View reviewed changes

Adding support to creating custom schemes

b1b302a

jakecraige closed this Sep 8, 2015

marcelofabri mentioned this pull request Feb 12, 2016

[01/03/2016] O mundo é mais que seu umbigo CocoaHeadsBrasil/equinociOS#13

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding support to creating custom schemes #225

Adding support to creating custom schemes #225

marcelofabri commented May 25, 2015

gfontenot commented Jun 11, 2015

marcelofabri commented Jun 12, 2015

marcelofabri commented Jul 27, 2015

bitcrumb commented Aug 10, 2015

jakecraige commented Aug 10, 2015

marcelofabri commented Aug 11, 2015

jakecraige Aug 11, 2015

marcelofabri Aug 11, 2015

jakecraige Aug 11, 2015

marcelofabri Aug 11, 2015

jakecraige Sep 4, 2015

jakecraige commented Aug 11, 2015

marcelofabri commented Aug 11, 2015

jakecraige commented Aug 11, 2015

jakecraige Aug 11, 2015

marcelofabri Aug 11, 2015

gfontenot commented Aug 11, 2015

marcelofabri commented Aug 11, 2015

jakecraige Aug 11, 2015

jakecraige commented Aug 11, 2015

borwahs commented Aug 11, 2015

marcelofabri commented Aug 12, 2015

borwahs commented Aug 12, 2015

marcelofabri commented Aug 28, 2015

marcelofabri commented Aug 29, 2015

jakecraige commented Aug 29, 2015

jakecraige Sep 4, 2015

marcelofabri Sep 4, 2015

marcelofabri commented Sep 4, 2015

jakecraige Sep 4, 2015

marcelofabri Sep 4, 2015

jakecraige commented Sep 4, 2015

marcelofabri commented Sep 4, 2015

jakecraige commented Sep 8, 2015

Adding support to creating custom schemes #225

Adding support to creating custom schemes #225

Conversation

marcelofabri commented May 25, 2015

gfontenot commented Jun 11, 2015

marcelofabri commented Jun 12, 2015

marcelofabri commented Jul 27, 2015

bitcrumb commented Aug 10, 2015

jakecraige commented Aug 10, 2015

marcelofabri commented Aug 11, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakecraige commented Aug 11, 2015

marcelofabri commented Aug 11, 2015

jakecraige commented Aug 11, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gfontenot commented Aug 11, 2015

marcelofabri commented Aug 11, 2015

Choose a reason for hiding this comment

jakecraige commented Aug 11, 2015

borwahs commented Aug 11, 2015

marcelofabri commented Aug 12, 2015

borwahs commented Aug 12, 2015

marcelofabri commented Aug 28, 2015

marcelofabri commented Aug 29, 2015

jakecraige commented Aug 29, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcelofabri commented Sep 4, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakecraige commented Sep 4, 2015

marcelofabri commented Sep 4, 2015

jakecraige commented Sep 8, 2015