Refactor class_eval with string into class_eval with block

[rdvdijk]

This commit changes the "class_eval with string"-style to "class_eval with block"-style.

The only worry I have is the remaining string eval in the method definition on line 96:

              define_method :scope_condition do
                eval "%{#{configuration[:scope]}}"
              end

Of course this eval happens in the current code, but just isn't that obvious as it is now.

Added bonus is that the position column is now quoted lazily, removing the need of an established database connection at model load-time, as mentioned in #214.

(Now let's see if all supported Ruby versions agree with this, Travis will let us know soon 😉)

[rdvdijk]

Travis build fails because:

1. github_changelog_generator depends on rack, which will install the latest version 2.0.1, which in turn requires Ruby >= 2.2.2
2. acts_as_list depends on activerecord >= 3.0, which will install the latest version 5.0.0, which depends on activesupport version 5.0.0, which also depends on Ruby >= 2.2.2

I can fix the first by requiring rack < 2.0 in the Gemfile, but what about the second problem?.

(Nevermind the second point, just noticed the use of appraisal.)

EDIT: I've proposed a fix to the Travis build in #216

[brendon]

Thanks for this @rdvdijk. Let's take a look at this once we've sorted out the rack problem :)

[brendon]

Hi @rdvdijk, can you rebase this to master (I've fixed the tests), and see how the suite runs?

[rdvdijk]

@brendon Rebased. I left the tab-to-spaces fix in the Gemfile 😉

Thanks for improving upon my appraisals/rack PR.

[brendon]

Thanks @rdvdijk :) All seems to have passed.

@swanandp, @fabn, and @krzysiek1507 did you want to have any input into this one?

[swanandp]

Somehow this name doesn't sound good. Maybe caller_class ?

[brendon]

Agreed :)

[rdvdijk]

Maybe caller_class ?

Sounds good. I'll change it.

[swanandp]

Good stuff! Just left a minor comment.

[brendon]

Ok, once that's fixed, I'll merge this one.

[brendon]

Oh, also needs rebasing :)

[rdvdijk]

Ah, feedback! I've been away on holiday, I'll take a look at the comments soon. And do another rebase. 😄

[rdvdijk]

@brendon I've fixed the variable name and rebased.

The quoted_position_column_with_table_name change made the rebase a little challenging, but I think all is well now. (I did notice that that there were no explicit tests for that particular feature...)

[brendon]

Thanks @rdvdijk, I think the reason why there were no explicit tests was that it was just a refactor of existing code, though for sure, they could have attempted to test for their obscure failure case. I might ping them on that. I'm happy to merge this now. Thank you for all your hard work on this.

[rdvdijk]

Awesome, thanks!

About my initial comment about the remaining string-eval in scope condition: it might be time to deprecate unsafe code like that. I think the "symbol" way of providing a scope condition should suffice in all use cases. Or are there use cases that I'm just not seeing here?

[brendon]

Let's open this up in a new issue. :)