Move SourceAnnotationExtractor under Rails module #32065
Conversation
|
https://github.com/search?l=Ruby&q=SourceAnnotationExtractor+-filename%3Aannotations.rake&type=Code seems to show some usage, so I think it's worth a deprecation. |
|
@matthewd sounds good. So deprecate the constant in 6.0, then remove it in 6.1? |
|
Yup. Rename stays the same; we just add in a http://api.rubyonrails.org/classes/ActiveSupport/Deprecation/DeprecatedConstantProxy.html for the old name. |
|
@matthewd sorry for the delay — I just updated the PR with two things:
Please let me know if you have any feedback. Thanks! |
This class should be under Rails module as it belongs to Rails.
This cleans up the documentation for SourceAnnotationExtractor because RDoc does not seems to know how to parse `Struct.new() do` block.
sikachu
force-pushed
the
move-SourceAnnotationExtractor-under-rails-namespec
branch
from
934093c
to
a252858
Compare
1.2.1 fixes a bug in `Kernel.require` and resolve a test failure. See Shopify/bootsnap#143
sikachu
force-pushed
the
move-SourceAnnotationExtractor-under-rails-namespec
branch
from
a252858
to
f2ebfcb
Compare
|
I added a commit to update bootsnap to the latest version to fix the issue with |
|
Thanks, @sikachu! |
|
RSpec Rails is broken by this change because it accesses Is there a way we can avoid breaking |
|
😦 interesting. I'm sorry for breaking your code. Let me try to find a way to work around that. I assume that this only happen when |
|
http://api.rubyonrails.org/classes/ActiveSupport/Deprecation/DeprecatedConstantAccessor.html exists for that purpose, but I don't like the idea of using it at the top level 😕 Maybe we can make DeprecatedConstantProxy inherit from Module, and then define both |
Right 👍 I discovered the problem when testing my application against Rails master, which I'm only doing to try to flush out regressions in Rails. I'm not blocked by it, no apology necessary 🙂
This does appear to work! 💥 |
|
Just want to drop a note that I'm now working on this to make sure that nobody else would run into the same issue as @eugeneius 😅 I'm currently trying to fix |
With this code (exctracted from derailed_benchmarks): ```ruby require "bundler/inline" gemfile(true) do source "https://rubygems.org" gem "rails", "~> 6.0.0.rc1", require: false end FILES = [ "rails/engine/configuration", "rails/source_annotation_extractor", "active_support/deprecation" ] module Kernel alias :original_require :require def require(file) Kernel.require(file) end class << self alias :original_require :require end end Kernel.define_singleton_method(:require) do |file| original_require(file) end FILES.each do |file| puts "requiring file: #{file}" require file end ``` It fails with Rails 6 and the change introduced by 32065 ``` requiring file: rails/engine/configuration requiring file: rails/source_annotation_extractor Traceback (most recent call last): 11: from repro_derailed.rb:33:in `<main>' 10: from repro_derailed.rb:33:in `each' 9: from repro_derailed.rb:35:in `block in <main>' 8: from repro_derailed.rb:21:in `require' 7: from repro_derailed.rb:30:in `block in <main>' 6: from repro_derailed.rb:30:in `require' 5: from /Users/benoit.tigeot/.rvm/gems/ruby-2.5.1/gems/railties-6.0.0.rc1/lib/rails/source_annotation_extractor.rb:8:in `<top (required)>' 4: from /Users/benoit.tigeot/.rvm/gems/ruby-2.5.1/gems/activesupport-6.0.0.rc1/lib/active_support/deprecation/proxy_wrappers.rb:10:in `new' 3: from /Users/benoit.tigeot/.rvm/gems/ruby-2.5.1/gems/activesupport-6.0.0.rc1/lib/active_support/deprecation/proxy_wrappers.rb:10:in `new' 2: from /Users/benoit.tigeot/.rvm/gems/ruby-2.5.1/gems/activesupport-6.0.0.rc1/lib/active_support/deprecation/proxy_wrappers.rb:125:in `initialize' 1: from /Users/benoit.tigeot/.rvm/gems/ruby-2.5.1/gems/activesupport-6.0.0.rc1/lib/active_support/deprecation/proxy_wrappers.rb:23:in `method_missing' /Users/benoit.tigeot/.rvm/gems/ruby-2.5.1/gems/activesupport-6.0.0.rc1/lib/active_support/deprecation/proxy_wrappers.rb:148:in `warn': private method `warn' called for nil:NilClass (NoMethodError) ``` Related: - zombocom/derailed_benchmarks#130 - #32065
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]
This is pretty much just a housekeeping PR.
I was browsing http://edgeapi.rubyonrails.org/ and notices that this class is listed on the left size with no namespace prefix. Given that this class belongs to Rails framework, and that it's already in
railties/lib/railshierarchy, I believe it's best to move this class down intoRailsmodule.In some sense, I believe that this could be considered a breaking change. However, given that user will access this class through rake task, I think this is fine to change.
Please let me know what you think.