Adding Back Rails::Engine::Railties#engines #8557

Merged
merged 1 commit into from Jan 22, 2013

Projects

None yet

6 participants

@timraymond
Contributor

First PR for me, so please let me know if I've done something horribly wrong :-P

Removing Rails::Engine::Railties#engines breaks functionality with gems such as Thinking Sphinx.
This restores it with a deprecation warning. See #8551

@pixeltrix
Member

@timraymond ❤️ - a few things:

  1. You need to also add the delegation to self.class as Thinking Sphinx calls the instance method not the class method.
  2. Can you add a test to ensure the method is deprecated
  3. Finally, can you add a CHANGELOG entry.

Thanks

@carlosantoniodasilva

Awesome @timraymond, thanks for contributing!

@rafaelfranca
Member

@timraymond ❤️

Also put Closes #8551 in your commit message and github will close that issue automatically.

@timraymond
Contributor

Wow! Didn't know you could do that. I'm away from my laptop for the
holidays, but I'll be back later this week and I'll finish this up.

Sent from my iPad

On Dec 21, 2012, at 12:14 PM, "Rafael Mendonça França" <
notifications@github.com> wrote:

@timraymond https://github.com/timraymond [image: ❤️]

Also put Closes #8551 in your commit message and github will close that
issue automatically.


Reply to this email directly or view it on
GitHubhttps://github.com/rails/rails/pull/8557#issuecomment-11625642.

@timraymond
Contributor

Done! I think it's been properly squashed and whatnot. Let me know if there's anything else.

@carlosantoniodasilva carlosantoniodasilva commented on an outdated diff Jan 3, 2013
railties/lib/rails/engine/railties.rb
+ attr_reader :_all
+
+ def initialize
+ @_all ||= ::Rails::Railtie.subclasses.map(&:instance) +
+ ::Rails::Engine.subclasses.map(&:instance)
+ end
+
+ def self.engines
+ @engines ||= ::Rails::Engine.subclasses.map(&:instance)
+ end
+
+ def each(*args, &block)
+ _all.each(*args, &block)
+ end
+
+ def - (others)
@carlosantoniodasilva
carlosantoniodasilva Jan 3, 2013 Member

I think the space after - is not necessary.

@carlosantoniodasilva carlosantoniodasilva commented on an outdated diff Jan 3, 2013
railties/lib/rails/engine/railties.rb
+ ::Rails::Engine.subclasses.map(&:instance)
+ end
+
+ def self.engines
+ @engines ||= ::Rails::Engine.subclasses.map(&:instance)
+ end
+
+ def each(*args, &block)
+ _all.each(*args, &block)
+ end
+
+ def - (others)
+ _all - others
+ end
+
+ delegate :engines, :to => "self.class"
@carlosantoniodasilva
carlosantoniodasilva Jan 3, 2013 Member

Mind using new hash style?

@timraymond timraymond Adding Back Rails::Engine::Railties#engines
Removing it breaks functionality with gems such as Thinking Sphinx.
This restores it with a deprecation warning. Closes #8551
48d5a33
@timraymond
Contributor

Cleaned up!

@pixeltrix pixeltrix was assigned Jan 5, 2013
@rafaelfranca
Member

@pixeltrix All yours

@rafaelfranca rafaelfranca was assigned Jan 21, 2013
@rafaelfranca rafaelfranca merged commit 48d5a33 into rails:master Jan 22, 2013
@spastorino
Member

I'm guessing that @timraymond did it this way because that's how it is in 3-2-stable:
https://github.com/rails/rails/blob/3-2-stable/railties/lib/rails/engine.rb#L445

It used to be like your gist until @drogus changed it in 32a5b49.

Member

Pushed 45aabe6

@davydotcom

Silly question, cant find an answer. Never got a deprecation warning in rails 4 about this method being gone. Now testing with 4.1.x and its failing. What is an alternative method to grab the list of engines.

@davydotcom

Figured it out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment