Skip to content

Conversation

@salmonthefish
Copy link

Currently, Qless::Job attempts to find constants by splitting the worker_class string (e.g., 'Jobs::GoogleMail::Backup::Master' becomes ['Jobs', 'GoogleMail', 'Backup', 'Master'] and looking up each successive constant within the scope of the previous constant. This works most of the time. However, if you have constants like A::B::C and B::X, given 'A::B::C', Qless will incorrectly return the B associated with B::X and throw a NameError trying to find C within that B. This restricts the way we can name things in CORE. Given the entire properly scoped class name, constantize doesn't run into this issue.

@jbodah
Copy link

jbodah commented Feb 23, 2015

allows Rails-style autoloading, lgtm

@ANorwell
Copy link

interesting stuff

@james-lawrence
Copy link

looking at the code I don't follow your logic on it incorrectly associating B with B::X when given 'A::B::X' given the fact its using reduce.

@jbodah jbodah force-pushed the use_constantize branch from 8f036cf to e97e594 Compare July 27, 2015 17:23
@jbodah
Copy link

jbodah commented Jul 27, 2015

@james-lawrence would this work better for you? uses ActiveSupport's constant lookup if ActiveSupport is defined

@tdg5
Copy link

tdg5 commented Aug 7, 2015

LGTM

@james-lawrence
Copy link

I'm still against it because no one has figured out WHY it breaks in the current form. so we are patching something that we don't understand. Instead of finding the underlying cause which could be causing other issues in our system we are ignoring.

@wr0ngway
Copy link

wr0ngway commented Aug 7, 2015

2.1.3 :001 > module B; module X; end; end
 => nil 
2.1.3 :004 > %w[A B C].reduce(Object) {|c, n| c.const_get(n) }
NameError: uninitialized constant A
2.1.3 :005 > module A; module Y; end; end
 => nil 
2.1.3 :006 > %w[A B C].reduce(Object) {|c, n| c.const_get(n) }
NameError: uninitialized constant C
2.1.3 :007 > %w[A B C].reduce(Object) {|c, n| c.const_get(n, false) }
NameError: uninitialized constant A::B

Looks like A.const_get(B) finds global B that is up the chain in the global scope. Giving inherit=false to const_Get seems to do what we want.
http://ruby-doc.org/core-2.1.0/Module.html#method-i-const_get

@tdg5
Copy link

tdg5 commented Aug 7, 2015

@wr0ngway nice find, I was just wondering if that's what was happening!

@jbodah
Copy link

jbodah commented Aug 7, 2015

@wr0ngway cool! Let me give that a shot, I think that's a nicer solution

@james-lawrence
Copy link

@wr0ngway Thanks for that find =)

@jbodah
Copy link

jbodah commented Aug 7, 2015

More explicitly what's happening from a pry-remote session. The fix for adding false that @wr0ngway suggested fixes the issue, refactoring to use that instead:

Frame number: 0/26

From: /Users/Bodah/repos/backupify/backupify/vendor/bundle/ruby/2.1.0/bundler/gems/qless-6ae4868846ea/lib/qless/job.rb @ line 24 Qless::BaseJob#klass:

    19: def klass
    20:   @klass ||= @klass_name.split('::').reduce(Object) do |context, name|
    21:     begin
    22:       context.const_get(name)
    23:     rescue LoadError => e
 => 24:       require 'rubygems'; require 'pry'; binding.remote_pry
    25:     end
    26:   end
    27: end

[1] pry(#<Qless::Job>)> @klass_name
=> "Jobs::Office365::Mail::Backup::EWS::Master"
[2] pry(#<Qless::Job>)> Object.const_get('Jobs')
=> Jobs
[3] pry(#<Qless::Job>)> _.const_get('Office365')
=> Jobs::Office365
[4] pry(#<Qless::Job>)> _.const_get('Mail')
=> Mail
[5] pry(#<Qless::Job>)> _.const_get('Backup')
RuntimeError: Circular dependency detected while autoloading constant Backup
from /Users/Bodah/repos/backupify/backupify/vendor/bundle/ruby/2.1.0/gems/activesupport-4.1.5/lib/active_support/dependencies.rb:478:in `load_missing_constant'
[6] pry(#<Qless::Job>)> Object.const_get('Jobs', false)
=> Jobs
[7] pry(#<Qless::Job>)> _.const_get('Office365', false)
=> Jobs::Office365
[8] pry(#<Qless::Job>)> _.const_get('Mail', false)
=> Jobs::Office365::Mail
[9] pry(#<Qless::Job>)> _.const_get('Backup', false)
=> Jobs::Office365::Mail::Backup
[10] pry(#<Qless::Job>)>

@james-lawrence
Copy link

LGTM

jbodah added a commit that referenced this pull request Aug 10, 2015
@jbodah jbodah merged commit e2f964b into master Aug 10, 2015
@jbodah jbodah deleted the use_constantize branch August 10, 2015 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants