Extract modules

[ledestin]

.acts_as_list was too long.

[brendon]

@swanandp, and @fabn, @ledestin has partitioned up the acts_as_list call. It's just a code reorganisation. Do you approve of these changes?

[fabn]

I personally like the idea of splitting the code in multiple files to have smaller pieces of code. The only concern about this is maybe that it contains too much meta programming, but current implementation has the same level of MP and I don't think is easy to avoid that.

[brendon]

@swanandp, your thoughts?

Also @ledestin, what do you think of placing all of the definers in a subdirectory called definers then removing definer from the file name and module name and then calling them say: ActiveRecord::Acts::List::Definers::Callback?

[brendon]

Thanks @fabn, yes, this is just organising the 'crap draw' basically without throwing anything away. I think because of the variable method names it's hard to avoid the class_eval stuff unfortunately. Though, there is possibly a case for using extend for those class methods that aren't variable. I asked Dmitry to look into it but I think he wanted to do this refactor first (is that right Dmitry?).

[ledestin]

@brendon hopefully, Definers::Callback should be enough. Should be ok. But I wouldn't do it, unless you really want to have them in a separate directory, as I prefer the way it reads now.

[ledestin]

@brendon it'd be hard to see what's changed (diff) after move to another file, so that's why I wanted to do this PR first.

Also, I reconsidered UpdatePositonMethodDefiner and now I think it should be merged into ColumnMethodDefiner, because those methods deal with position.

[brendon]

@ledestin, that's all good. Let's stick with the current arrangement. It's my habit that when there's a group of things, I like to put them in their own place and remove any repetition around their naming if possible. Your way does read cleaner though.

I'd probably use a plural for the class name if we went with that option: Definer::Callbacks. That still reads reasonably well, but then does that make it strange to have them in a directory named definers?

Merge those two classes you mentioned in your last message if you think it proper.

[ledestin]

@brendon we probably would have to have Definers:: module to match directory name. Better for consistence and is probably necessary for autoloading to work.

Going to merge.

[brendon]

Thanks @ledestin. Is this ready to merge?

[ledestin]

@brendon I've added a small change, I came up with later. Now it's ready.

[brendon]

Thanks @ledestin, I've merged this :)

[ledestin]

Great @brendon :)

[randoum]

@ledestin @brendon @fabn This Pr induced a bug

There was an error while trying to load the gem 'acts_as_list'. (Bundler::GemRequireError) Gem Load Error is: uninitialized constant ActiveRecord::Acts

In the stack Kernel.require loads /lib/acts_as_list.rb, which in turn loads lib/acts_as_list/active_record/acts/column_method_definer.rb. First line of column_method_definer.rb is module ActiveRecord::Acts::List::ColumnMethodDefiner but at that point ruby knows nothing about ActiveRecord::Acts

A fix would be to split the module definition in the following pattern:

module ActiveRecord
  module Acts
    module List
      module ColumnMethodDefiner
        ...
      end
    end
  end
end

The same should be done for all the definers created in this PR

acts_as_list/active_record/acts/column_method_definer
acts_as_list/active_record/acts/scope_method_definer
acts_as_list/active_record/acts/top_of_list_method_definer
acts_as_list/active_record/acts/add_new_at_method_definer
acts_as_list/active_record/acts/aux_method_definer
acts_as_list/active_record/acts/callback_definer

[brendon]

Hi @randoum, just checking you're getting this on master? I changed how acts_as_list loads itself here: #240

I remember getting a similar error due to the relative class references ColumnMethodDefiner etc... That's why they're absolute in the patch.

Can you provide an environment that triggers your bug as I haven't seen it and the suite runs green. Is it a bit random?

[randoum]

Yes on master. Does not show in the tests. You should reproduce in any projects if I you to your Gemfile

gem 'acts_as_list', git: 'https://github.com/swanandp/acts_as_list.git'

Then try:

$ rails c
/opt/rbenv/versions/2.3.3/lib/ruby/gems/2.3.0/gems/bundler-1.13.7/lib/bundler/runtime.rb:94:in `rescue in block (2 levels) in require': There was an error while trying to load the gem 'acts_as_list'. (Bundler::GemRequireError)
Gem Load Error is: uninitialized constant ActiveRecord::Acts
Backtrace for gem load error is:
/opt/rbenv/versions/2.3.3/lib/ruby/gems/2.3.0/bundler/gems/acts_as_list-83abef9f25ed/lib/acts_as_list/active_record/acts/column_method_definer.rb:1:in `<top (required)>'
/opt/rbenv/versions/2.3.3/lib/ruby/gems/2.3.0/bundler/gems/acts_as_list-83abef9f25ed/lib/acts_as_list.rb:1:in `<top (required)>'
/opt/rbenv/versions/2.3.3/lib/ruby/gems/2.3.0/gems/bundler-1.13.7/lib/bundler/runtime.rb:91:in `require'

Solution is as I previously posted

[randoum]

Note that you may have to load your model class in order to see the error message, so in the console for example:

irb(main):001:0> TodoList

[brendon]

Thanks @randoum, that makes sense. I've tweaked the load order on master. Can you give that a go and let me know? It doesn't hurt to do this since there's no code being run anymore in list.rb.

[randoum]

Yep, that fixed it. Cheers

[brendon]

Good to hear :)