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

Remove unnecessary comments in cache_helper.rb [ci skip] #27760

Merged

Conversation

kenta-s
Copy link
Contributor

@kenta-s kenta-s commented Jan 21, 2017

I looked into a TODO comment in cache_helper.rb.

I think we can remove those comments.

the comments were added in this commit: 9de8305

Below is a pic of the commit opened in RubyMine.
screen shot 2017-01-21 at 8 05 41 pm

I guess he left the comment because he thought capture method would be able to work by its own without 'the dance' in his future.

I git-reseted to see the code at the time. The methods named capture were in ActionView::Helpers::CaptureHelper(which exists in current master branch, too) and Rails::Generators::TestCase. in this case, it is obvious that he meant the first one.

Now, we can use capture not only in erb, but builder and jbuilder with no problem even without write_fragment_for.

From these reasons, those long lived comments can go.

To be honest, I'm not 100% sure the intention of his comments.
Please let me know if there is someone who knows what he meant, and who thinks I'm wrong. If so, I want to try to deal with it.

Personally, I don't think it is a good idea to ignore TODO comments such a long time(6 years in this case).

@rails-bot
Copy link

r? @rafaelfranca

(@rails-bot has picked a reviewer for you, use r? to override)

@kaspth
Copy link
Contributor

kaspth commented Jan 21, 2017

Funnily enough, we did run into this last year on rails/jbuilder#341 (comment).

But definitely agreed that we may as well remove the comments. Thanks for the investigation!

@kaspth kaspth merged commit fb5bd3c into rails:master Jan 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants