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.

Sign up for GitHub

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

Added 'pre' and 'post' setup blocks #174

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Added 'pre' and 'post' setup blocks #174

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2c6db30
added 'pre' and 'post' setup blocks
colinta Oct 14, 2014
ae4579e
oh shoot, I forgot to copy in this refactor
colinta Oct 15, 2014
936b9fe
using 'pre_setup_blocks' we can set 'app.files' before the Rakefile s…
colinta Oct 15, 2014
176f291
this was calling 'setup' twice, since 'config' also calls 'setup'
colinta Oct 15, 2014
7f766b8
Merge branch 'master' into pre_post_setup_blocks
colinta Jan 16, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion lib/motion/project/app.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,16 @@ def builder
end

def setup(&block)
config.setup_blocks << block
config_without_setup.setup_blocks << block
config.setup
end
Copy link
Member Author

Choose a reason for hiding this comment

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

calling config here is what causes some troubles, because config ends up determining sdk and api versions. For instance, on android, I need to set app.api_version = '19', otherwise I get an error about the "L" SDK not being installed.

Which is fine until I include a gem, which calls setup, which calls config, and goes looking for the api_version. My Rakefile's setup hasn't been called, so it detects the wrong one, and errors out.


def pre_setup(&block)
config_without_setup.setup_blocks << block
end
Copy link
Member Author

Choose a reason for hiding this comment

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

pre_setup uses the same @setup_blocks as setup, but doesn't call config.setup at the end.


def post_setup(&block)
config_without_setup.post_setup_blocks << block
Copy link
Member Author

Choose a reason for hiding this comment

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

post_setup on the other hand uses a new ivar, since the Rakefile setup needs to be called in between the pre and post setup blocks.

Ok, so all that said, it wouldn't be any trouble to use a more explicit @pre_setup = [] array of blocks. There's no advantage other than being explicit, though.

end

def build(platform, opts={})
Expand Down
25 changes: 23 additions & 2 deletions lib/motion/project/config.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,8 @@ def self.make(template, project_dir, build_mode)

def initialize(project_dir, build_mode)
@project_dir = project_dir
@files = Dir.glob(File.join(project_dir, 'app/**/*.rb'))
@files = []
@app_dir = 'app'
Copy link
Member Author

Choose a reason for hiding this comment

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

This is, to me, the most controversial change. Previously in the setup block, the app.files has already been set, but this delays that until after all setup blocks are called.

The reason for the change is so that gems, using pre_setup, can append their files to app.files. Right now they have to be inserted before the app files, otherwise dependencies get very messy... Ideally, to keep things identical, app.files would be set before the Rakefile's setup block is called.

Oh, shoot, I just realized that using @pre_setup blocks would allow for this... OK one sec I'm gonna do that change.

@build_mode = build_mode
@name = 'Untitled'
@resources_dirs = [File.join(project_dir, 'resources')]
Expand Down Expand Up @@ -105,12 +106,32 @@ def setup_blocks
@setup_blocks ||= []
end

def post_setup_blocks
@post_setup_blocks ||= []
end

def setup
if @setup_blocks
@setup_blocks.each { |b| b.call(self) }
@setup_blocks = nil
end

if @post_setup_blocks
@post_setup_blocks.each { |b| b.call(self) }
end

# should we include a check here for "already included app/ files"? it's
# not necessary since this operation is idempotent.
Copy link
Member

Choose a reason for hiding this comment

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

I’m unclear as to why not to check it, especially since you do it after all :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh shoot, I just realized I refactored this in /Library/RubyMotion, and didn't copy that in. I'm gonna push that up (with comments to hopefully answer some of these questions)

Dir.glob(File.join(project_dir, "#{@app_dir}/**/*.rb")).each do |app_file|
@files << app_file unless @files.include?(app_file)
end

if @setup_blocks || @post_setup_blocks
Copy link
Member

Choose a reason for hiding this comment

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

Can you leave a comment as to why this is not needed with pre_setup blocks?

Copy link
Member Author

Choose a reason for hiding this comment

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

pre_setup blocks are stored in @setup_blocks

validate
end

@setup_blocks = nil
@post_setup_blocks = nil
Copy link
Member

Choose a reason for hiding this comment

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

Why is this needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Existing gems will be calling setup, we need to make sure not to keep calling the same blocks.

Also, this is a carryover - see line 111 of previous version.


self
end

Expand Down