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.

Sign up for GitHub

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

Closed
wants to merge 4 commits into from
Closed

Misc touchups #150

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
61ec878
Have less activerecord::base envy
bf4 Jul 23, 2019
edeaac6
Always call super from included
bf4 Jul 23, 2019
fb06561
Make --only-failures possible
bf4 Jul 23, 2019
a6c31af
Remove trailing whitespace
bf4 Jul 24, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,5 @@ pkg/*

Gemfile.lock
*.gemfile.lock

spec/examples.txt
7 changes: 6 additions & 1 deletion lib/ranked-model.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ class NonNilColumnDefault < StandardError; end
MIN_RANK_VALUE = -2147483648

def self.included base
super
Copy link
Author

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

Copy link
Owner

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?

Copy link
Author

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

Copy link
Owner

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? :)

Copy link
Owner

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? :)

Copy link
Owner

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?

Copy link
Author

@bf4 bf4 Jan 19, 2021 •
edited
Loading

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

Copy link
Owner

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?

Copy link
Author

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

Copy link
Owner

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?


base.class_eval do
class_attribute :rankers
Expand Down Expand Up @@ -42,6 +43,10 @@ def ranker name
end
end

def ranker_connection_class
ActiveRecord::Base
Copy link
Owner

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? :)

Copy link
Author

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

Copy link
Owner

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.

end

private

def ranks *args
Expand Down Expand Up @@ -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?
Copy link
Owner

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.

Copy link
Author

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

end

end
Expand Down
2 changes: 1 addition & 1 deletion lib/ranked-model/railtie.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
module RankedModel

class Railtie < Rails::Railtie

initializer "ranked-model.initialize" do |app|

end
Expand Down
2 changes: 1 addition & 1 deletion lib/ranked-model/ranker.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Owner

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.

Copy link
Author

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

Copy link
Owner

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.

# If there is room at the bottom of the list and we're added to the very top of the list...
if current_first.rank && current_first.rank > RankedModel::MIN_RANK_VALUE && rank == RankedModel::MAX_RANK_VALUE
# ...then move everyone else down 1 to make room for us at the end
Expand Down
2 changes: 2 additions & 0 deletions spec/spec_helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@

config.order = :random
Kernel.srand config.seed

config.example_status_persistence_file_path = "spec/examples.txt"
end

RSpec::Matchers.define :define_constant do |expected|
Expand Down