-
-
Notifications
You must be signed in to change notification settings - Fork 133
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
Misc touchups #150
Misc touchups #150
Conversation
not sure why this failed |
@@ -11,6 +11,7 @@ class NonNilColumnDefault < StandardError; end | |||
MIN_RANK_VALUE = -2147483648 | |||
|
|||
def self.included base | |||
super |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is really a good idea to have. oh well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm unfamiliar with this technique. Can you link me to something that explains why it's necessary or nice to have?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, it may be unexpected but it makes sense, too. It's just Ruby
ruby <<EOF
class Foo
attr_reader :bar
def self.inherited(base)
puts "I am in Foo: #{base}"
end
end
class Bar < Foo
def self.inherited(base)
puts "I am in Bar: #{base}"
end
end
class Baz < Bar
def self.inherited(base)
puts "I am in Baz: #{base}"
end
end
EOF
prints
I am in Foo: Bar
I am in Bar: Baz
while
ruby <<EOF
class Foo
attr_reader :bar
def self.inherited(base)
super
puts "I am in Foo: #{base}"
end
end
class Bar < Foo
def self.inherited(base)
super
puts "I am in Bar: #{base}"
end
end
class Baz < Bar
def self.inherited(base)
super
puts "I am in Baz: #{base}"
end
end
EOF
prints
I am in Foo: Bar
I am in Foo: Baz
I am in Bar: Baz
and
ruby <<EOF
class Foo
attr_reader :bar
def self.inherited(base)
puts "I am in Foo: #{base}"
super
end
end
class Bar < Foo
def self.inherited(base)
puts "I am in Bar: #{base}"
super
end
end
class Baz < Bar
def self.inherited(base)
puts "I am in Baz: #{base}"
super
end
end
EOF
prints
I am in Foo: Bar
I am in Bar: Baz
I am in Foo: Baz
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your examples are around self.inherited
but this method is self.included
. Am I missing something? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So to clarify, this is just in case someone chooses to extend this module for some reason? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hehe, that's what I was thinking. Not sure why anyone would want to extend RankedModel. And is it possible to extend a module?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can include or prepend a module into a module. I started to write an example but it worked in your favor in this context :) the only way to inherit a module is by fooling Ruby because Module is a class
class SomeModule < Module
end
# something or other, I forget why
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lol, I'm glad we've managed to nut this one out :) So I guess remove this line from the PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess there's rubocop/ruby-style-guide#809 rubocop/rubocop#8376 rubocop/rubocop@9d5cbe6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep sounds like they don't raise anything on module included
based on my quick skim of that code. I assume that's what you were saying?
Are you happy to remove this and also look at calling connection
directly on the class per our discussions below and see how it tests out?
@@ -11,6 +11,7 @@ class NonNilColumnDefault < StandardError; end | |||
MIN_RANK_VALUE = -2147483648 | |||
|
|||
def self.included base | |||
super |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm unfamiliar with this technique. Can you link me to something that explains why it's necessary or nice to have?
@@ -42,6 +43,10 @@ def ranker name | |||
end | |||
end | |||
|
|||
def ranker_connection_class | |||
ActiveRecord::Base |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We explicitly rely on ActiveRecord so why do we need to abstract this? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for application with multiple databases it's nice to be able to specify the connection class. ideally really it would just be the current active record class which would always do the right thing due to the way the connection handler threading is handled, but having a way to easily specify the connection class is probably best in both cases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may not need this per my other comment below.
@@ -183,7 +183,7 @@ def assure_unique_position | |||
|
|||
def rearrange_ranks | |||
_scope = finder | |||
escaped_column = ActiveRecord::Base.connection.quote_column_name ranker.column | |||
escaped_column = instance_class.ranker_connection_class.connection.quote_column_name ranker.column |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This call becomes quite clunky with the abstraction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it should probably just be instance_class anyhow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. Let's go with that. Especially given a particular model may have a different connection.
@@ -69,7 +74,7 @@ def ranks *args | |||
end | |||
|
|||
def column_default ranker | |||
column_defaults[ranker.name.to_s] if ActiveRecord::Base.connected? | |||
column_defaults[ranker.name.to_s] if ranker_connection_class.connected? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it work if we just called connected?
on the current class? Then we don't have to worry about ranker_connection_class
at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on the right receiver, yeah.. rails connection handlers are per thread class attributes
Just me using the wrong example... modules are inheritance anyhow.
Rails calls super too fwiw
…On Sun, Jan 17, 2021, 4:15 PM Brendon Muir ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In lib/ranked-model.rb
<#150 (comment)>:
> @@ -11,6 +11,7 @@ class NonNilColumnDefault < StandardError; end
MIN_RANK_VALUE = -2147483648
def self.included base
+ super
Your examples are around self.inherited but this method is self.included.
Am I missing something? :)
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
<#150 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABC4QWHLTTECGGBAGG55O3S2NOQDANCNFSM4IGQ2QRQ>
.
|
Extracted from #148