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

Support multiple exclusion options #3

Merged
merged 5 commits into from
Nov 30, 2016

Conversation

kato2222
Copy link
Contributor

@kato2222 kato2222 commented Nov 18, 2016

I fixed issue #2.

Multiple exclusion options are not support by environment variable.
Please review this.

code example:

lane :run_synx do
   synx exclusion: 'Foo'
  synx exclusion: ['Foo', 'Bar']
end

@kirakik
Copy link

kirakik commented Nov 23, 2016

@afonsograca would be great if you could review this...

@afonsograca
Copy link
Owner

hey @tero2222,
Thank you for contributing!
Sorry for the delay, I went on holidays the last 2 weeks and was completely offline. I will take a look and merge it ASAP

cmd << ["--exclusion #{params[:exclusion]}"] if params[:exclusion]
if params[:exclusion]
Array(params[:exclusion]).each {|exclusion|
cmd << ["--exclusion #{exclusion}"] }
Copy link
Owner

Choose a reason for hiding this comment

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

@tero2222 would your solution work for pathnames with spaces in it? don't you think it would be better if you enclose the exclusion in quotes? So that the command would look like this:
$ synx -p --exclusion "/OCMock/Core Mocks" Source/OCMock.xcodeproj/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@afonsograca Thanks for review!
I think that it would be better to use Shellwords.join().

Example

Shellwords.join([
  "synx",
  "-p",
  "--exclusion",
  "/OCMock/Core Mocks",
  "Source/OCMock.xcodeproj/"])
#=> synx -p --exclusion /OCMock/Core\ Mocks Source/OCMock.xcodeproj/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I fixed to use shellwords.

d2d10c0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And separated key and value of exclusion option.

3d3aaed

Copy link
Owner

Choose a reason for hiding this comment

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

cool! it looks neat!
Just a final remark, can you replace the curly braces {} with do...end on the each loop? Just to keep the ruby code style conventions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!
I fixed the coding style.

3619e0b

@afonsograca afonsograca merged commit d753e00 into afonsograca:master Nov 30, 2016
@kato2222 kato2222 deleted the support-multiple-exclusion branch November 30, 2016 11:56
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