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

Fix the type of option reinstall_app in Screengrab tool #7980

Merged
merged 1 commit into from
Jan 24, 2017
Merged

Fix the type of option reinstall_app in Screengrab tool #7980

merged 1 commit into from
Jan 24, 2017

Conversation

gersonmdesouza
Copy link
Contributor

Checklist

  • I've run bundle exec rspec from the root directory to see all new and existing tests pass
  • I've followed the fastlane code style and run bundle exec rubocop -a to ensure the code style is valid
  • I've read the Contribution Guidelines
  • I've updated the documentation if necessary.

Description

Add the property is_string with value false to reinstall_app Screengrab options that need to have it.

Motivation and Context

This Screengrab option is a flag and need to be boolean type. If the property is_string is not set to false, Fastlane doesn't recognize it as boolean and crash with the error:

'reinstall_app' value must be a String! Found TrueClass instead.

@gersonmdesouza
Copy link
Contributor Author

@hjanuschka Hey, I open a Pull Request (#7923) that adds reinstall_app flag to Screengrab. In the file options.rb I copied the block of code of the flag exit_on_test_failure and change it to fit my new flag. But now, when I run Fastlane Screengrab, there is an error with this flag.

'reinstall_app' value must be a String! Found TrueClass instead.

So, I open this Pull Request adding is_string with value false. Can you tell me why the option exit_on_test_failure doesn't have it, and why I need to put it on reinstall_app?

@mfurtak
Copy link
Contributor

mfurtak commented Jan 24, 2017

@gersonmendes Thanks for putting this together.

Your fix looks correct to me, and my belief is that the other option is incorrect as well. As to the question of why the other is not causing a problem... I'm not sure without investigating more deeply.

I'm going to accept your change, and then we can make a new PR for the other option. 👍

@mfurtak mfurtak merged commit 5285ee0 into fastlane:master Jan 24, 2017
@gersonmdesouza
Copy link
Contributor Author

@mfurtak Thanks for the reply.

I will try to investigate exit_on_test_failure flag, but I agree with you, the other option seems to be incorrect as well. I can create a new PR fixing the other option if you want, just let me know :)

@hjanuschka
Copy link
Collaborator

i think exit_on_test_failure is faulty too - but it works (seems to) as it has a default_value:true
but a pr to improve it would be nice.

@mfurtak
Copy link
Contributor

mfurtak commented Jan 24, 2017

I've adjusted the other option here: #7981

@gersonmdesouza gersonmdesouza deleted the fix-screengrab-options branch January 24, 2017 21:34
@KrauseFx KrauseFx mentioned this pull request Jan 24, 2017
@fastlane fastlane locked and limited conversation to collaborators Apr 25, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants