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

Modify HttpPositionalArguments to use session instead of headers #5865

Merged
merged 2 commits into from
May 14, 2018

Conversation

rrosenblum
Copy link
Contributor

I have been working on upgrading a Rails project and started getting errors about headers not being a valid keyword argument. It looks like sessionis supposed to be used.

https://github.com/rails/rails/blob/master/actionpack/lib/action_controller/test_case.rb#L460

@bbatsov
Copy link
Collaborator

bbatsov commented May 9, 2018

Hmm, I wonder if something changed between Rails 4 and 5.

@rrosenblum
Copy link
Contributor Author

It doesn't look like anything changed between 4 and 5 regarding headers vs session. Here is the implementation from 4.2, https://github.com/rails/rails/blob/4-2-stable/actionpack/lib/action_controller/test_case.rb#L595.

# Example sending parameters, +nil+ session and setting a flash message:
#   
#   process :view, 'GET', { id: 7 }, nil, { notice: 'This is flash message' }

I'm surprised that I seem to be the first person that has run into this issue. I looked at the implementation for Rails 5.0 and 5.1 and both of those are using session as well.

@rrosenblum rrosenblum force-pushed the http_positional_args_session branch from 08e3467 to 856fc01 Compare May 10, 2018 13:01
@bbatsov bbatsov merged commit 8f311a3 into rubocop:master May 14, 2018
@bbatsov
Copy link
Collaborator

bbatsov commented May 14, 2018

Me too. That's why I asked - I assumed someone would have noticed this problem by now.

Anyways, I'll take your word for this! Thanks! 👍

@rrosenblum
Copy link
Contributor Author

I found where headers likely came from ActionDispatch uses headers https://github.com/rails/rails/blob/master/actionpack/lib/action_dispatch/testing/integration.rb#L210.

There is likely some other work that should be done to this cop to make it work a bit better. xhr aren't handled by this. Also, format inside of parameters needs to be converted to the as keyword argument.

Even with some of the bugs, this cop is invaluable for upgrading Rails.

@rrosenblum rrosenblum deleted the http_positional_args_session branch July 3, 2018 18:23
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.

None yet

2 participants