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

Add bang version to OrderedOptions #20208

Merged
merged 1 commit into from May 26, 2015

Conversation

Projects
None yet
5 participants
@gaurish
Contributor

gaurish commented May 19, 2015

Every method should have a bang counter-part. if someone uses a bang version & its not defined. it will raise an ArgumentError.

example(normal version)
Rails.application.secrets.big_query

bang version
Rails.application.secrets.big_query!

By:
Aditya Sanghi(@asanghi)
Gaurish Sharma(@gaurish)

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca May 19, 2015

Member

Thank you for the pull request.

Every method should have a bang counter-part.

Not true. There is no rule about this in any place and even if it is a rule, there is cases where it doesn't make sense.

I'm having problem to understand where this would be useful, could you give us an example?

Member

rafaelfranca commented May 19, 2015

Thank you for the pull request.

Every method should have a bang counter-part.

Not true. There is no rule about this in any place and even if it is a rule, there is cases where it doesn't make sense.

I'm having problem to understand where this would be useful, could you give us an example?

@asanghi

This comment has been minimized.

Show comment
Hide comment
@asanghi

asanghi May 20, 2015

Contributor

@rafaelfranca Absolutely right, there is no such rule.

secrets.yml is more likely than not to contain passwords/keys to services which might be essential to your application's execution. Without those keys the application is unlikely to work as expected (or crash in unexpected ways). The bang version of the method basically allows us to crash in a more expected way, letting us know that a blank has been encountered when it's not expected. In absence of this, we're more likely to write custom code checking for blanks and handling it with custom error methods (and then possibly erroring out anyway).

Case in point

if (slack_url = Rails.application.secrets.slack_url).present?
  $slack_notifier = Slack::Notifier.new(slack_url)
else
  Rails.logger.error("Unable to find slack_url in secrets.yml")
  raise "Unable to find slack_url in secrets.yml"
end

gets replaced by

  $slack_notifier = Slack::Notifier.new(Rails.application.secrets.slack_url!)

This is a similar style to .save!, and .create! methods where ! indicates that an exception might be raised by this method call.

What do you think?

Contributor

asanghi commented May 20, 2015

@rafaelfranca Absolutely right, there is no such rule.

secrets.yml is more likely than not to contain passwords/keys to services which might be essential to your application's execution. Without those keys the application is unlikely to work as expected (or crash in unexpected ways). The bang version of the method basically allows us to crash in a more expected way, letting us know that a blank has been encountered when it's not expected. In absence of this, we're more likely to write custom code checking for blanks and handling it with custom error methods (and then possibly erroring out anyway).

Case in point

if (slack_url = Rails.application.secrets.slack_url).present?
  $slack_notifier = Slack::Notifier.new(slack_url)
else
  Rails.logger.error("Unable to find slack_url in secrets.yml")
  raise "Unable to find slack_url in secrets.yml"
end

gets replaced by

  $slack_notifier = Slack::Notifier.new(Rails.application.secrets.slack_url!)

This is a similar style to .save!, and .create! methods where ! indicates that an exception might be raised by this method call.

What do you think?

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca May 20, 2015

Member

@asanghi thanks for the explanation.

@gaurish Could you add a CHANGELOG entry?

Member

rafaelfranca commented May 20, 2015

@asanghi thanks for the explanation.

@gaurish Could you add a CHANGELOG entry?

@gaurish

This comment has been minimized.

Show comment
Hide comment
@gaurish

gaurish May 23, 2015

Contributor

PR Updated according to feedback given.

Contributor

gaurish commented May 23, 2015

PR Updated according to feedback given.

Add bang version to OrderedOptions
By:
Aditya Sanghi(@asanghi)
Gaurish Sharma(gaurish)
@asanghi

This comment has been minimized.

Show comment
Hide comment
@asanghi

asanghi May 26, 2015

Contributor

/cc @rafaelfranca seems okay now?

Contributor

asanghi commented May 26, 2015

/cc @rafaelfranca seems okay now?

@rafaelfranca rafaelfranca merged commit e768c51 into rails:master May 26, 2015

rafaelfranca added a commit that referenced this pull request May 26, 2015

* Add a bang version to `ActiveSupport::OrderedOptions` get methods which will raise an `KeyError` if the value is `.blank?`
Before:
if (slack_url = Rails.application.secrets.slack_url).present?)

This comment has been minimized.

@chopmo

chopmo Jun 7, 2015

Contributor

Small nitpick: The last closing parens should be deleted.

@chopmo

chopmo Jun 7, 2015

Contributor

Small nitpick: The last closing parens should be deleted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment