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

Deprecate Module.local_constants #23936

Merged
merged 1 commit into from
Mar 1, 2016

Conversation

yui-knk
Copy link
Contributor

@yui-knk yui-knk commented Feb 28, 2016

Module.local_constants is treated as public API in 'Constans' session
of Rails guide active_support_core_extensions. So it is better to remove
#:nodoc:.

@rails-bot
Copy link

r? @arthurnn

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

@matthewd
Copy link
Member

That was a 1.8 compatibility method. We should remove it from the guide and deprecate it.

@yui-knk
Copy link
Contributor Author

yui-knk commented Feb 28, 2016

I'm not sure what '1.8 compatibility method' means.
But if Module.local_constants is intended to used by only internal Rails codes, I agree to remove it from the guide (and update this PR 😍 )

@matthewd
Copy link
Member

In ruby 1.8, it was not so easy. Now, it shouldn't be used anywhere; we should change anything that does to just call constants(false) directly.

@yui-knk yui-knk force-pushed the local_constants_to_be_public branch from 2aea5ad to 86d4e18 Compare March 1, 2016 11:34
After Ruby 1.9, we can easily get the constants that have been
defined locally by `Module.constants(false)`.
@yui-knk yui-knk changed the title [ci skip] Make Module.local_constants to be public API Deprecate Module.local_constants Mar 1, 2016
@yui-knk
Copy link
Contributor Author

yui-knk commented Mar 1, 2016

@matthewd Updated!

@rafaelfranca rafaelfranca merged commit 86d4e18 into rails:master Mar 1, 2016
rafaelfranca added a commit that referenced this pull request Mar 1, 2016
@yui-knk yui-knk deleted the local_constants_to_be_public branch March 2, 2016 01:53
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.

None yet

6 participants