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

Code cleanup for ActionDispatch::Flash#call #10585

Closed
wants to merge 1 commit into from
Closed

Code cleanup for ActionDispatch::Flash#call #10585

wants to merge 1 commit into from

Conversation

julianvargasalvarez
Copy link
Contributor

Here I include a fix for the broken specs in railties as reported here
#10580 (comment)

Here I include a fix for the broken specs in railties as reported here
#10580 (comment)
@julianvargasalvarez
Copy link
Contributor Author

@guilleiguaran I ran the tests for Railties and actionpack and evenything is passing.

@rafaelfranca
Copy link
Member

What happened with the env[KEY]?

@guilleiguaran
Copy link
Member

Thank you

/cc @rubys how are your tests with this?

@julianvargasalvarez
Copy link
Contributor Author

In line 244 the variable flash_hash was created with env[KEY] and then later inside the if that key was rewritten with flash_hash

@rafaelfranca
Copy link
Member

I'm not seeing the key being rewritten

@julianvargasalvarez
Copy link
Contributor Author

It was before. Check this out c43ca06#L0R245

in line 244 flash_hash = env[KEY] then in lines 249 and 241 new_hash = flash_hash and thin in line 254 env[KEY] = new_hash

@rafaelfranca
Copy link
Member

Ok. But why it was removed?

@julianvargasalvarez
Copy link
Contributor Author

Because that code seems to be unnecesary, I ran the tests for railties and actionpack and both are passing

@rubys
Copy link
Contributor

rubys commented May 13, 2013

/cc @rubys how are your tests with this?

👍

@rafaelfranca
Copy link
Member

@julianvargasalvarez I'm not sure that code is unnecessary. The test coverage can not be accurate. I prefer to be conservative and merge the @arunagw fix. Thank you

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

4 participants