Make sure that valid? raises rather than verify #9171
Conversation
| def verify!(value) | ||
| UI.user_error!("Invalid value '#{value}' for option '#{self}'") unless valid?(value) | ||
| true | ||
| true if valid?(value) |
There was a problem hiding this comment.
I think I find this a little bit confusing, I think - we have a method ending with a "!" that doesn't explicitly have a raise in it, and a method ending with a "?" that returns either true or else raises an exception.
Feels like the entire body of valid? should be moved into verify!, and the true return value should be eliminated (since it is always true if not raising).
Or am I missing something? 😄
There was a problem hiding this comment.
Yes, technically the correct thing to do would be to raise the exception here (method with the !), and only return booleans in valid?. However this would also mean that we can't show specific error messages that are helpful for the user to understand. Maybe we should just have one method that raises the exception?
|
Hey @ohayon 👋 Thank you for your contribution to fastlane and congrats on getting this pull request merged 🎉 Please let us know if this change requires an immediate release by adding a comment here 👍 |
fastlane-bot
added
status: included-in-next-release
and removed
status: included-in-next-release
labels
|
Congratulations! 🎉 This was released as part of fastlane 2.31.0 🚀 |
Saw a crash in Stackdriver that showed a lot of failures in the
verifymethod, but it calls intovalid?which should be responsible for raising exceptions.