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

Fix problem with accessing deprecated constant proxy's subclass #36557

Merged

Conversation

sikachu
Copy link
Member

@sikachu sikachu commented Jun 26, 2019

This commit fixes #36313.

After #32065 moved SourceAnnotationExtractor into Rails module, it broke the ability to access SourceAnnotationExtractor::Annotate directly as user would get this error:

TypeError: Rails::SourceAnnotationExtractor is not a class/module

This commit fixes the issue by making DeprecatedConstantProxy to inherit from Module and then defines method_missing and const_missing to retain the previous functionality.

Thank you @matthewd for the idea of how to fix the issue!

@sikachu sikachu added this to the 6.0.0 milestone Jun 26, 2019
@sikachu sikachu changed the title Fix problem with accessing constant proxy subclass Fix problem with accessing deprecated constant proxy's subclass Jun 26, 2019
@sikachu sikachu force-pushed the fix-source-annotation-extractor-annotation branch from dbd3260 to 7bac1e9 Compare July 3, 2019 12:13
@sikachu
Copy link
Member Author

sikachu commented Jul 3, 2019

@matthewd I use @deprecator.warn just like in method_missing, and use const_get as your feedback. I also added another test just to make sure we get both NameError and deprecation message when someone tries to access a constant that isn't exist on the new constant.

Would you mind taking another look and merge it if you think it's good to go?

@sikachu sikachu force-pushed the fix-source-annotation-extractor-annotation branch from 7bac1e9 to 09b9d37 Compare July 5, 2019 03:22
@sikachu
Copy link
Member Author

sikachu commented Jul 5, 2019

I removed the new method. Ready for another look!

This commit fixes rails#36313.

After rails#32065 moved `SourceAnnotationExtractor` into `Rails` module, it
broke the ability to access `SourceAnnotationExtractor::Annotate`
directly as user would get this error:

    TypeError: Rails::SourceAnnotationExtractor is not a class/module

This commit fixes the issue by making `DeprecatedConstantProxy` to
inherit from `Module` and then defines `method_missing` and
`const_missing` to retain the previous functionality.

Thank you Matthew Draper for the idea of how to fix the issue!

[Prem Sichanugrist & Matthew Draper]
@sikachu sikachu force-pushed the fix-source-annotation-extractor-annotation branch from 09b9d37 to 48c0abb Compare July 5, 2019 03:41
@sikachu
Copy link
Member Author

sikachu commented Jul 5, 2019

@matthewd actually, removing that cause this test to fail (Waffles is set to false in this case):

def test_deprecated_constant_proxy_doesnt_wrap_falsy_objects
proxy = ActiveSupport::Deprecation::DeprecatedConstantProxy.new(Waffles, NewWaffles)
assert_not proxy
end

So, I've brought back that self.new method to retain the functionality.

@matthewd matthewd merged commit 5a9301c into rails:master Jul 16, 2019
@matthewd
Copy link
Member

I'll merge because this has been waiting on me for ages (🙈), but I think that's a bad test (this class expects to receive strings containing the old/new constant names, not actual constant references) and can be deleted.

@sikachu sikachu deleted the fix-source-annotation-extractor-annotation branch July 16, 2019 04:22
rafaelfranca pushed a commit that referenced this pull request Jul 16, 2019
…r-annotation

Fix problem with accessing deprecated constant proxy's subclass
@natevick
Copy link
Contributor

🎉thanks to you all!

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.

Rails 6.0.0.rc1 Missing Deprecation SourceAnnotationExtractor::Annotation
3 participants