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 ActionController::ModelNaming #18194

Merged

Conversation

@claudiob
Copy link
Member

claudiob commented Dec 24, 2014

The methods in these modules are not used anywhere.
They used to be invoked in polymorphic_routes.rb but their last usage was removed in e821045.

What is your opinion about removing these methods?
They do belong to the public API, but in reality their code has already been duplicated into ActionView::ModelNaming.

The ActionView module is still used (by dom_id and dom_classto associate records with DOM elements).
The ActionController module is not.

Please tell me if you think that removing this module is a good idea and if you feel like the PR is okay as it is, or whether some deprecation warning should be displayed.

The methods in these modules are not used anywhere. They used to be
invoked in polymorphic_routes.rb but their usage was removed in e821045.

What is your opinion about removing these methods?

They do belong to the public API, but in reality their code has already been duplicated to ActionView::ModelNaming, since they are used by methods like `dom_id` and `dom_class` to associated records with DOM elements (in
ActionView).

Please tell me if you think that removing this module is a good idea and,
in that case, if the PR is okay as it is, or you'd rather start by showing
a deprecation message, and remove the module in Rails 5.1.
@sgrif
Copy link
Contributor

sgrif commented Dec 25, 2014

We need some form of deprecation warning. Also go spend time with your family, this shit can wait. 😁

@arthurnn
Copy link
Member

arthurnn commented Dec 25, 2014

merry xmas!!!

@claudiob
Copy link
Member Author

claudiob commented Dec 25, 2014

Merry 🎄 to you all! 🎅

@guilleiguaran
Copy link
Member

guilleiguaran commented Dec 26, 2014

Sorry, this was never supposed to be public, I forgot to mark it as private when I added it in 166dbaa

I've looking on Google and Github to check if someone really is using one of the methods inside of AC::Naming and I'm pretty sure no one is doing it and since 5.0 is a major release I think is reasonable to remove this without deprecation.

guilleiguaran added a commit that referenced this pull request Dec 26, 2014
…l-naming

Remove ActionController::ModelNaming
@guilleiguaran guilleiguaran merged commit 122f55c into rails:master Dec 26, 2014
1 check passed
1 check passed
continuous-integration/travis-ci The Travis CI build passed
Details
@claudiob claudiob deleted the claudiob:remove-action-controller-model-naming branch Dec 26, 2014
@rafaelfranca
Copy link
Member

rafaelfranca commented Dec 29, 2014

👍

prathamesh-sonpatki added a commit to prathamesh-sonpatki/rails that referenced this pull request Jun 7, 2016
prathamesh-sonpatki added a commit to prathamesh-sonpatki/rails that referenced this pull request Jun 7, 2016
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

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