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

Gemfile #28

Merged
merged 8 commits into from
Mar 10, 2017
Merged

Gemfile #28

merged 8 commits into from
Mar 10, 2017

Conversation

djbe
Copy link
Member

@djbe djbe commented Mar 1, 2017

switch to using a gemfile and calling via bundle

@djbe djbe force-pushed the feature/gemfile branch from b113453 to efe39f2 Compare March 1, 2017 23:57
task :compile => :modules do |task|
Utils.print_info 'Compiling template output files'

exit Dir.glob('Tests/Expected/**/*.swift').map { |f|
Copy link
Contributor

Choose a reason for hiding this comment

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

exit typically takes an integer (the exit code) as argument, right?

So if one of the compile_file(f, task) returns false, the reduce will return false and exit false would be interpreted as exit(0)… generally meaning no error?
While if all compile_file(f, task) return true, thereduce will return true and exit true would be interpreted as exit(1)… meaning an error?

I think it would be better if compile_file returned the return code / status code of the execution of the Utils.run call (line 87), and doing a exit_codes = Dir.glob(…).map { compile_file(…) } first, then do a return exit_codes.find { |c| c != 0 } || 0 to globally return the first non-zero exit code, or exit(0) if all exit codes are 0.

Copy link
Member Author

Choose a reason for hiding this comment

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

The argument can be a boolean or an integer. If it's boolean, then true means success. If it's an integer than 0 means success.

Copy link
Contributor

Choose a reason for hiding this comment

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

TIL 👍

subtask = File.basename(f, '.*')

begin
Utils.run(commands, task, subtask, xcrun: true)
Copy link
Contributor

Choose a reason for hiding this comment

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

As stated above, Utils.run should hopefully return the status code of the ran command, so the same way as it's done on line 63, the function here should just end with the call to Utils.run(…) without the need of a begin/rescue 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.

Problem with that is that it can fail so we need to catch the exception.

rakelib/pod.rake Outdated
task :lint do |task|
Utils.print_info 'Linting the pod spec'
Utils.run(%Q(pod lib lint "#{POD_NAME}.podspec" --quick), task)
if File.file?('Podfile')
Copy link
Contributor

Choose a reason for hiding this comment

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

Given the nature of the task, we should rather test the presence of a #{PODNAME}.podspec rather than the presence of a Podfile.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do

Rakefile Outdated
@@ -6,7 +6,6 @@ WORKSPACE = 'StencilSwiftKit'
TARGET_NAME = 'Tests'
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we document somewhere (comment at the top of each rakelib/*.rake file? Elsewhere?) what global variables are used by various tasks, and thus which variables are expected to be defined in each repo's Rakefile?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea.

Copy link
Contributor

Choose a reason for hiding this comment

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

Besides, SCHEME_NAME would be a better name for that variable 😉

@djbe djbe force-pushed the feature/gemfile branch from 0899e0c to 203d5e2 Compare March 4, 2017 17:22
@djbe djbe merged commit 73b0114 into master Mar 10, 2017
@djbe djbe deleted the feature/gemfile branch March 10, 2017 21:15
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.

2 participants