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 - Class#superclass_delegating_accessor #14271

Merged
merged 1 commit into from
Mar 20, 2014

Conversation

akshay-vishnoi
Copy link
Contributor

class TestClass
  superclass_delegating_accessor :test_attribute
end

# Before
TestClass.private_methods.include?(:_test_attribute) # => false
TestClass.private_methods.include?(:_test_attribute=) # => false

# After
TestClass.private_methods.include?(:_test_attribute) # => true
TestClass.private_methods.include?(:_test_attribute=) # => true

@akshay-vishnoi
Copy link
Contributor Author

Any update on this?

@akshay-vishnoi
Copy link
Contributor Author

cc: @chancancode @rafaelfranca

@chancancode
Copy link
Member

Sorry, I must be missing some context here. What's the bug here? I am not familiar with this method and it has no docs. Is that even public API? Can you fill me in? Thanks 😄

@akshay-vishnoi
Copy link
Contributor Author

@chancancode:

  1. Bug - Comment in Class#superclass_delegating_accessor says that this method creates private _name and _name= methods, which previously was not true. Refer here.

    Also test cases were not covering the whole functionality of Class#superclass_delegating_accessor.

  2. Yes it is in public API.

@akshay-vishnoi
Copy link
Contributor Author

@chancancode Any update on this? 😄

@chancancode
Copy link
Member

I still don't know what this method does =/

If it's a public API we should add docs? If not we should :nodoc it? You don't have to do those, but I don't feel comfortable merging this without understanding what this does. Maybe someone else more familiar with this can help you then :)

@akshay-vishnoi
Copy link
Contributor Author

cc @rafaelfranca @wycats

@akshay-vishnoi
Copy link
Contributor Author

Any update on this?

@wycats
Copy link
Member

wycats commented Mar 10, 2014

I'll take a look today.

@akshay-vishnoi
Copy link
Contributor Author

Thanks @wycats

@akshay-vishnoi
Copy link
Contributor Author

Any update on this?

@thedarkone
Copy link
Contributor

I don't think superclass_delegating_accessor is used by Rails anymore, it also has no docs. So how about removing altogether?

@carlosantoniodasilva
Copy link
Member

Definitely +1 for deprecating this.

@wycats
Copy link
Member

wycats commented Mar 12, 2014

I'm pretty sure this was an earlier experiment before we settled on class_attribute. I see no reason to keep it around if it's not used anywhere.

@akshay-vishnoi
Copy link
Contributor Author

Should I update the PR?
That means updated PR will include:

  1. Remove activesupport/lib/active_support/core_ext/class/delegating_attributes.rb and activesupport/test/core_ext/class/delegating_attributes_test.rb
  2. Remove the require statements from active_support/core_ext/class.rb and activerecord/lib/active_record/base.rb.

@carlosantoniodasilva
Copy link
Member

Please do, just add a deprecation now for master/4.2, and we remove in the next version. It's not gonna be a problem for Rails, but other libs might be using it in ways we don't know, so deprecating first is always better.

@akshay-vishnoi
Copy link
Contributor Author

Okay I will add deprecation warning and corresponding test cases.
But I think we should update the code for master/4.2 because

  1. Previous code was not correct.
  2. Previous test cases were incorrectly written. (If we are not updating the code, but we should atleast cover all test cases.)

cc @carlosantoniodasilva, @wycats, @thedarkone - Any thoughts?

@carlosantoniodasilva
Copy link
Member

I think it's not necessary to change code/tests we're going to rm, specially because we haven't received issues related to this piece of code in a long time as far as I remember. We can just go ahead and deprecate it as is. Thanks!

@akshay-vishnoi
Copy link
Contributor Author

Sorry for late reply. I will do it today. Thanks for help!!!

@akshay-vishnoi
Copy link
Contributor Author

@carlosantoniodasilva Please check the updated code.

@carlosantoniodasilva
Copy link
Member

Can you add a test that shows it's deprecated, and a changelog entry? It may also need to require the related deprecation file. Thanks.

@akshay-vishnoi
Copy link
Contributor Author

@carlosantoniodasilva: Please check updated code.

  1. Test case added
  2. CHANGELOG.md updated
  3. deprecation file required

Thanks

@robin850
Copy link
Member

@akshay-vishnoi : Could you please rebase your branch ? It doesn't apply cleanly. Also it would be nice if you could update the PR title and description accordingly. Thanks for your contribution! 👍

@akshay-vishnoi akshay-vishnoi changed the title Bug Fix - Class#superclass_delegating_accessor Deprecate - Class#superclass_delegating_accessor Mar 19, 2014
@akshay-vishnoi
Copy link
Contributor Author

@robin850

  1. I have done rebasing. Please check updated code.
  2. Also changed the title of PR.
  3. Should I change my comment as well, as description of PR is now shown as a comment?

@carlosantoniodasilva
Copy link
Member

Looks good, however I think now other tests will show warnings when running the suite, maybe we should silence those?

Thanks @akshay-vishnoi

@rafaelfranca
Copy link
Member

If we are going to deprecate we need to tell our users what they have to use instead. I searched on GitHub and there are a lot of ocorrences of this method https://github.com/search?p=1&q=superclass_delegating_accessor&type=Code

@akshay-vishnoi
Copy link
Contributor Author

@rafaelfranca I am also working on it, I can extract a gem for this. So we can refer to it if necessary.

@thedarkone
Copy link
Contributor

@akshay-vishnoi the deprecation message should refer users to class_attribute.

If we are going to deprecate we need to tell our users what they have to use instead. I searched on GitHub and there are a lot of ocorrences of this method https://github.com/search?p=1&q=superclass_delegating_accessor&type=Code

@rafaelfranca wow, that's a lot of code... but clicking through random pages, it all looks to be either vendored Rails code or an old (also vendored) version of active_merchant.

@akshay-vishnoi
Copy link
Contributor Author

@rafaelfranca : Could you please help, actually when I am running test cases it is showing the warning and full backtrace, but when I am using rails as a gem then it is not showing the backtrace but only warning. What could be the possible reason? Although all test cases are running and there is no error or failure, but I am not getting how to stop these backtraces.

@rafaelfranca
Copy link
Member

You can use assert_deprecated or silence_warnings

@akshay-vishnoi
Copy link
Contributor Author

I have already used assert_deprecated and backtrace is not coming for this test, and if i have to stop it for all other test cases then i will need to change it at each place where Class#superclass_delegating_accessor is getting used.
I also noticed that ActiveSupport::Deprecation.debug is true here. Backtrace is coming due to this reason. If I set ActiveSupport::Deprecation.debug to false, then only warning message is coming, like other methods i.e. without any backtrace. Do you have any idea about this?
Thanks

@akshay-vishnoi
Copy link
Contributor Author

Okay it has been set in abstract_unit.rb. But how can we escape the deprecation warning for this line or similar to this line? As these lines are not the part of the test cases.

@rafaelfranca
Copy link
Member

I think you can use silence_warnings for that case.

@akshay-vishnoi
Copy link
Contributor Author

@rafaelfranca : Please check updated code. Will this work?

@akshay-vishnoi
Copy link
Contributor Author

@rafaelfranca : Any updates?

@@ -1,3 +1,7 @@
* Deprecate `Class#superclass_delegating_accessor`.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can also add a comment to use class_attribute instead in this changelog entry.

@carlosantoniodasilva
Copy link
Member

@akshay-vishnoi just a minor comment on the changelog, and then we're good to go.

@akshay-vishnoi
Copy link
Contributor Author

@carlosantoniodasilva Ya sure, Updating!!!!

@akshay-vishnoi
Copy link
Contributor Author

@carlosantoniodasilva : Code updated Please check.
When are we expecting next stable release, so that I can make another PR removing the same from the codebase?

carlosantoniodasilva added a commit that referenced this pull request Mar 20, 2014
Deprecate superclass_delegating_accessor, use class_attribute instead.
@carlosantoniodasilva carlosantoniodasilva merged commit a2a7f8b into rails:master Mar 20, 2014
@carlosantoniodasilva
Copy link
Member

Thanks.

@carlosantoniodasilva
Copy link
Member

@akshay-vishnoi this is only going to be released on 4.2, which is going to take a while (considering 4.1 is in rc state), so keep it in your todo list :)

@akshay-vishnoi
Copy link
Contributor Author

Ohkay Thanks 💚

akshay-vishnoi pushed a commit to akshay-vishnoi/rails that referenced this pull request May 4, 2014
akshay-vishnoi pushed a commit to akshay-vishnoi/rails that referenced this pull request Sep 17, 2014
akshay-vishnoi pushed a commit to akshay-vishnoi/rails that referenced this pull request Sep 17, 2014
amccloud added a commit to amccloud/active_merchant that referenced this pull request Sep 28, 2014
Class#superclass_delegating_accessor was deprecated in Rails 4.2.0

rails/rails#14271
lulalala added a commit to lulalala/active_merchant that referenced this pull request Oct 10, 2014
larrylv added a commit to larrylv/active_merchant that referenced this pull request Apr 21, 2015
Class#superclass_delegating_accessor from Rails is now deprecated.

rails/rails#14271

And uses of `superclass_delegating_accessor` use has been removed in #ffca0ca615bfa7bc99f8faae9b4ae95514a6604d and #ae02c93aaa52e4a63326d4b399bd8a9dd3977ed5
larrylv added a commit to larrylv/active_merchant that referenced this pull request Apr 21, 2015
Class#superclass_delegating_accessor from Rails is now deprecated.

rails/rails#14271

And uses of `superclass_delegating_accessor` use has been removed in #ffca0ca615bfa7bc99f8faae9b4ae95514a6604d and #ae02c93aaa52e4a63326d4b399bd8a9dd3977ed5
larrylv added a commit to larrylv/active_merchant that referenced this pull request Apr 30, 2015
Class#superclass_delegating_accessor from Rails is now deprecated.

rails/rails#14271

And uses of `superclass_delegating_accessor` use has been removed in #ffca0ca615bfa7bc99f8faae9b4ae95514a6604d and #ae02c93aaa52e4a63326d4b399bd8a9dd3977ed5
akshay-vishnoi pushed a commit to akshay-vishnoi/rails that referenced this pull request May 24, 2015
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.

7 participants