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 RecordTagHelper #18411

Merged
merged 1 commit into from Feb 18, 2015
Merged

Remove RecordTagHelper #18411

merged 1 commit into from Feb 18, 2015

Conversation

@todd
Copy link
Member

todd commented Jan 8, 2015

Per DHH in #18337, I've extracted ActionView::Helpers::RecordTagHelper to an external gem (source currently lives at todd/record_tag_helper). I also added extraction notices for anyone upgrading who was using these methods - the corresponding code and tests should probably be fully removed at the next minor release after 5.0.0.

Regarding the gem, I still need to write some documentation (the README is kinda sparse at the moment) and I'd appreciate someone else going over the code to make sure I'm not missing anything, though it seems pretty straightforward. There's also the question of whether the source for record_tag_helper should stay under my own account or get forked into the Rails org - I don't have a strong opinion either way. The answer to that question should also dictate who publishes the gem, so I'm holding off on pushing anything to RubyGems.

expected = %(<li class="record_tag_post" id="record_tag_post_45"></li>)
actual = content_tag_for(:li, @post)
assert_dom_equal expected, actual
assert_raises(NoMethodError) { content_tag_for(:li, @post) }
end

This comment has been minimized.

@carlosantoniodasilva

carlosantoniodasilva Jan 8, 2015 Member

I think you can keep only 1 test for each method, should be enough.

This comment has been minimized.

@todd

todd Jan 8, 2015 Author Member

Yeah, I figured that would probably be enough, but I wanted to cover all the cases. I'll slim this down.

@carlosantoniodasilva
carlosantoniodasilva reviewed Jan 8, 2015
View changes
actionview/CHANGELOG.md Outdated
* Extracted `ActionView::Helpers::RecordTagHelper` to external gem
(`record_tag_helper`) and added removal notices.

*Todd Bealmear*

This comment has been minimized.

@carlosantoniodasilva

carlosantoniodasilva Jan 8, 2015 Member

This should be moved to the top.

This comment has been minimized.

@todd

todd Jan 8, 2015 Author Member

Wasn't sure about the ordering, I'll bump it up.

@egilburg
Copy link
Contributor

egilburg commented Jan 8, 2015

Public API changes should go through deprecation process

@carlosantoniodasilva
Copy link
Member

carlosantoniodasilva commented Jan 8, 2015

@egilburg please read the discussion on the related issue, linked above. It's going to be extracted to a gem.

@egilburg
Copy link
Contributor

egilburg commented Jan 8, 2015

@carlosantoniodasilva I did read about the gem extraction. In such cases, do we expect users to manually notice their app is broken (e.g. first time they get a runtime error) and add the gem entry, or would they get some runtime warnings first?

I also do realize we have changelogs and upgrade instructions. But I didn't know there is a deprecation difference between code which requires a gem change from code that requires any other equally simple change (e.g. change method call signature somewhere in the app)

@carlosantoniodasilva
Copy link
Member

carlosantoniodasilva commented Jan 8, 2015

The functionality will still be available for anyone who wants to use it. I don't believe people will just upgrade to Rails 5 and put their apps in production right away to see it blowing up. In short, this is pretty much the same we did with respond_to/respond_with and the responders gem, which is totally fine as long as there's an upgrade path imo.

@robertomiranda
Copy link
Contributor

robertomiranda commented Jan 11, 2015

record_tag_helper should be live under /rails?

@carlosantoniodasilva
Copy link
Member

carlosantoniodasilva commented Jan 11, 2015

We will have it under rails for now, as we generally do with other extractions at the beginning.

Per DHH in #18337, ActionView::Helpers::RecordTagHelper has been
extracted to an external gem (source currently lives at
todd/record_tag_helper). Removal notices have also been added for anyone
upgrading that use the extracted methods.
@rafaelfranca
Copy link
Member

rafaelfranca commented Feb 18, 2015

@todd could you transfer the repository to rails organization? I'll make sure you will have full access to it.

@todd
Copy link
Member Author

todd commented Feb 18, 2015

@rafaelfranca I'm not an admin for the Rails org, so I can't transfer ownership to Rails. Can I add you as an admin on the repo and have you transfer ownership?

@rafaelfranca
Copy link
Member

rafaelfranca commented Feb 18, 2015

@todd 👍

@todd
Copy link
Member Author

todd commented Feb 18, 2015

@rafaelfranca I added you as a collaborator, but I'm not sure if that grants you the ability to transfer repo ownership. It doesn't appear there's a way for me to edit your permissions on the repo beyond that. Let me know if we need to figure something else out.

@rafaelfranca
Copy link
Member

rafaelfranca commented Feb 18, 2015

@todd 😅 I think you will have to transfer for me and so I can transfer for rails.

@todd
Copy link
Member Author

todd commented Feb 18, 2015

@rafaelfranca Sheesh. Alright, done. 😛

rafaelfranca added a commit that referenced this pull request Feb 18, 2015
Remove RecordTagHelper
@rafaelfranca rafaelfranca merged commit 4ffe46f into rails:master Feb 18, 2015
1 check passed
1 check passed
continuous-integration/travis-ci The Travis CI build passed
Details
@rafaelfranca
Copy link
Member

rafaelfranca commented Feb 18, 2015

You should have access to the gem. Thank you so much for working on this @todd

@todd
Copy link
Member Author

todd commented Feb 18, 2015

@rafaelfranca My pleasure. What should the strategy/process look like for actually releasing the gem?

@rafaelfranca
Copy link
Member

rafaelfranca commented Feb 18, 2015

We usually release it with the beta release of Rails.

@todd
Copy link
Member Author

todd commented Feb 18, 2015

Ok, should I be responsible for that or will someone from the Core Team take the lead? Either way is fine, just trying to scope out my role moving forward.

@todd todd deleted the todd:extract_record_tag_helper branch Feb 18, 2015
@rafaelfranca
Copy link
Member

rafaelfranca commented Feb 18, 2015

@todd I think for the first release it is better to us handle, but you will have access to push the gem too.

@todd
Copy link
Member Author

todd commented Feb 18, 2015

👍 Sounds good.

@claudiob
Copy link
Member

claudiob commented Feb 20, 2015

Since this is now out of Rails, it would be great to have a running version of the gem on RubyGems as soon as possible, for people who want to transition.

To avoid squatting the record_tag_helper name, I blocked it on RubyGems by pushing a "0.0.0" version. All the common gem owners of Rails project are now owners of the record_tag_helper gem as well, so you can push a new version as soon as it's ready.

yhirano55 added a commit to yhirano55/rails that referenced this pull request Apr 7, 2018
* Since rails#18411, we started to inform about extracted gem (record_tag_helper)
  to developers who use `ActionView::Helpers::RecordTagHelper` 's methods.

* Currently, it seems no problem that we don't have to support no longer.
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.