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

Deprecate passing hash as first parameter into ActionController::Head #20407

Merged
merged 1 commit into from Jun 15, 2015

Conversation

@meinac
Copy link
Contributor

meinac commented Jun 1, 2015

Deprecate passing hash as first parameter into ActionController::Head head method
By deprecating this, also implicit status code will be deprecated too.
See #20372 for more information.
/cc @rafaelfranca

@kaspth
Copy link
Member

kaspth commented Jun 1, 2015

Shouldn't the hidden default :ok be deprecated?

@rafaelfranca
Copy link
Member

rafaelfranca commented Jun 1, 2015

Shouldn't the hidden default :ok be deprecated?

yes

@meinac
Copy link
Contributor Author

meinac commented Jun 1, 2015

I've deprecated both; passing hash as first parameter and default status code for this method. I asked this @rafaelfranca and he said deprecate both.

@kaspth
Copy link
Member

kaspth commented Jun 1, 2015

Shouldn't the status key be deprecated too then?

@meinac
Copy link
Contributor Author

meinac commented Jun 1, 2015

I've already deprecated passing Hash so passing :status as a key also deprecated.

@kaspth
Copy link
Member

kaspth commented Jun 1, 2015

Ah sorry, I didn't realize the code had been updated more.

@meinac
Copy link
Contributor Author

meinac commented Jun 1, 2015

@kaspth no problem bro 😄 what do you think about this pr now?

msg = "Passing Hash as first parameter"
options, status = status, nil
status ||= options.delete(:status) || (
(msg += " and default status code") && :ok

This comment has been minimized.

Copy link
@kaspth

kaspth Jun 2, 2015

Member

"Passing Hash as first parameter and default status code" doesn't make sense to me. What about and the implicit :ok status has been deprecated.?

It's okay to not have the code be completely DRY. I'd prefer making this more readable 😄

status ||= options.delete(:status) || (
(msg += " and default status code") && :ok
)
ActiveSupport::Deprecation.warn(msg + " for `head` is deprecated and will be removed in Rails 5.1. Please visit http://api.rubyonrails.org/classes/ActionController/Head.html for usage.")

This comment has been minimized.

Copy link
@kaspth

kaspth Jun 2, 2015

Member

Do we need a documentation link? cc @matthewd

This comment has been minimized.

Copy link
@meinac

meinac Jun 2, 2015

Author Contributor

I think yes 😄 but if you think it's not necessary I can remove it from message.

This comment has been minimized.

Copy link
@kaspth

kaspth Jun 2, 2015

Member

I just don't remember us doing that anywhere else. Besides they're calling head and you even mention it in the warning, so I'd rather ✂️ it.

@meinac
Copy link
Contributor Author

meinac commented Jun 2, 2015

@kaspth I've updated the pr.

status ||= options.delete(:status) || (
(msg += " and the implicit :ok status") && :ok
)
ActiveSupport::Deprecation.warn(msg + " for `head` method has been deprecated and will be removed in Rails 5.1. Please visit http://api.rubyonrails.org/classes/ActionController/Head.html for usage.")

This comment has been minimized.

Copy link
@kaspth

kaspth Jun 2, 2015

Member

We should wrap this to 80 chars, right @zzak?

This comment has been minimized.

Copy link
@meinac

meinac Jun 2, 2015

Author Contributor

@kaspth there are some deprecation warnings more then 200 line length.

@meinac
Copy link
Contributor Author

meinac commented Jun 2, 2015

@kaspth I've removed doc link from the deprecation message.

status ||= options.delete(:status) || (
(msg += " and the implicit :ok status") && :ok
)
ActiveSupport::Deprecation.warn(msg + " for `head` method has been deprecated and will be removed in Rails 5.1.")

This comment has been minimized.

Copy link
@kaspth

kaspth Jun 6, 2015

Member

Actually, I don't think we need method here.

@meinac
Copy link
Contributor Author

meinac commented Jun 6, 2015

@kaspth I've removed method from deprecation warning.

(msg += " and the implicit :ok status") && :ok
)
ActiveSupport::Deprecation.warn(msg + " for `head` has been deprecated and will be removed in Rails 5.1.")
end

This comment has been minimized.

Copy link
@kaspth

kaspth Jun 6, 2015

Member

Let's simplify this to:

if status.is_a?(Hash)
  msg = "Passing Hash as first parameter"
  msg << "and the implicit :ok status" unless status.key?(:status)
  options, status = status, status.delete(:status) || :ok

  ActiveSupport::Deprecation.warn(msg + "for `head`...")
end

This comment has been minimized.

Copy link
@kaspth

kaspth Jun 6, 2015

Member

Should also add a line break between the if status.is_a?(Hash) and location = options.delete(:location)

This comment has been minimized.

Copy link
@gsamokovarov

gsamokovarov Jun 6, 2015

Contributor

Just a minor nit as well, Hash#delete accepts a block, which can be used to set defaults.

E.g. status.delete(:status) { :ok }

@kaspth
Copy link
Member

kaspth commented Jun 6, 2015

@rafaelfranca once my final comments have been addressed, this is ready 👍

@meinac well done 👏

@meinac
Copy link
Contributor Author

meinac commented Jun 6, 2015

@kaspth done. Thanks 💛

if status.is_a?(Hash)
msg = "Passing Hash as first parameter"
msg << " and the implicit :ok status" unless status.key?(:status)
options, status = status, (status.delete(:status) || :ok)

This comment has been minimized.

Copy link
@kaspth

kaspth Jun 6, 2015

Member

As a tiny nitpick I don't like the parens here, I understand you're worried about the execution order. Would it help to follow @gsamokovarov's suggestion and use the block version?

@meinac
Copy link
Contributor Author

meinac commented Jun 6, 2015

👍

@zzak
Copy link
Member

zzak commented Jun 7, 2015

LGTM

@kaspth
Copy link
Member

kaspth commented Jun 7, 2015

I'd just hit me - we're should deprecate the status argument too, right?
head :ok, status: :no_content doesn't make sense to me.

@zzak
Copy link
Member

zzak commented Jun 7, 2015

@kaspth I thought that :no_content would be the 1st argument in this case?

@kaspth
Copy link
Member

kaspth commented Jun 7, 2015

Right, I guess what I'm trying to guard against is a non issue. Though the warning says nothing about how to comply with the changes, so we should add something like: Pass the status as the first argument instead.

@matthewd
Copy link
Member

matthewd commented Jun 7, 2015

I'll have a go at a more natural-sounding deprecation message after I get some sleep. But note there are two behaviour changes around nil status values here, neither of which seem to be intended... so we probably shouldn't merge as is.

@kaspth
Copy link
Member

kaspth commented Jun 9, 2015

@matthewd have you come up with anything? I'm mostly drawing blanks here.

@matthewd
Copy link
Member

matthewd commented Jun 9, 2015

How about:

(status[:status] ? "The :status option" : "The implicit :ok status") <<
  " on `head` has been deprecated and will be removed in Rails 5.1. Please pass the status as a separate parameter before the options, instead."

To elaborate on my previous comment, this seems to currently change the behaviour of both:

head(nil, {})
# and
head({ status: nil })

Unreasonable as those both may be, it seems unnecessary to break them while we're introducing a deprecation message.

@kaspth
Copy link
Member

kaspth commented Jun 9, 2015

Completely agree that we shouldn't break them and I think your deprecation message is spot on. @meinac, are you okay with that? 😄

@meinac
Copy link
Contributor Author

meinac commented Jun 9, 2015

@kaspth I'm ok with this suggestion, I've changed the message but the line takes a little bit long 😄 I hope this is not a problem because there are some deprecation messages longer than this one.

msg = status[:status] ? 'The :status option' : 'The implicit :ok status'
options, status = status, status.delete(:status) { :ok }

ActiveSupport::Deprecation.warn(msg + ' on `head` has been deprecated and will be removed in Rails 5.1. Please pass the status as a separate parameter before the options, instead.')

This comment has been minimized.

Copy link
@kaspth

kaspth Jun 9, 2015

Member

We could use squish here to great effect:

def print_deprecation_warning
ActiveSupport::Deprecation.warn(<<-MSG.squish)
Passing a class as a value in an Active Record query is deprecated and
will be removed. Pass a string instead.
MSG

@meinac
Copy link
Contributor Author

meinac commented Jun 9, 2015

@kaspth what about this one?

@kaspth
Copy link
Member

kaspth commented Jun 10, 2015

@meinac that's it! Lovely ❤️

@matthewd @rafaelfranca can one of you merge this?

@matthewd
Copy link
Member

matthewd commented Jun 10, 2015

Still doesn't address my concerns about nil statuses

@kaspth
Copy link
Member

kaspth commented Jun 10, 2015

True, I missed that. We should handle those gracefully too.

@meinac
Copy link
Contributor Author

meinac commented Jun 10, 2015

I think in this version we've covered the nil status situation. WDYT @kaspth , @matthewd ?

@matthewd
Copy link
Member

matthewd commented Jun 10, 2015

It technically won't give a warning for head(nil, {}), which we'll presumably break in 5.1... but I don't think that's something we ever supported. Just better that we do all the breaking together, rather than accidentally changing the one case now.

👍

@kaspth?

@kaspth
Copy link
Member

kaspth commented Jun 15, 2015

Sounds good, now you just need to rebase @meinac 😄

@meinac
Copy link
Contributor Author

meinac commented Jun 15, 2015

@kaspth done 👍

@kaspth
Copy link
Member

kaspth commented Jun 15, 2015

Great. @matthewd, you know what to do 😄

matthewd added a commit that referenced this pull request Jun 15, 2015
…ad_method

Deprecate passing hash as first parameter into ActionController::Head
@matthewd matthewd merged commit 57941f0 into rails:master Jun 15, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.