Use define_method when method name contains weird characters.

[KrzysiekJ]

In delegate_to_scoped_klass method in ActiveRecord, plain def was used when defining methods with weird characters in names, which resulted in syntax errors. I've changed it to use define_method instead and added a regression test for scope names with spaces.

This could be also rewritten to use a block instead of a string in module_eval, but in pull request #9206 I've read that it may impose some performance penalty, so I didn't change it as I don't know how important it is.

[sikachu]

I can see how that would break, but ... would you tell me why are you having a scope method which contains space? I don't think this is a general use case here as people would normally do method_with_underscore.

[lexmag]

👎

[rafaelfranca]

I don't think this is something we should support

[mhuggins]

Definite 👎. Why do this?

scope :"title containing space", -> { where("title LIKE '% %'") }

When you could just do this?

scope :title_containing_underscores, -> { where("title LIKE '% %'") }

All the example documentation regarding scope uses symbols with underscores for scope names.

[sikachu]

FTR: I'm actually -1 on this one, but want to see the use case first.

[KrzysiekJ]

Imagine the following model:

class Ticket < ActiveRecord::Base
  @priorities = ["not important", "minor", "major", "very important"]

  class << self
    def valid_priority? priority
      @priorities.include? priority
    end
  end
end

Then I may want to ease filtering by those priorites, so I write the following code:

class Ticket
  @priorities.each do |priority|
    scope priority, where(:priority => priority)
  end
end

This allows me to do things like

@tickets = @tickets.send params[:priority] if Ticket.valid_priority? params[:priority]

without caring about string processing of priority names in the views and controllers. It may occur that those scope names won't be hand-typed anywhere in the code, but if there is such need, it still can be done with send or by employing method_missing to convert dashes to spaces.

The above probably isn't a very important and convincing use case, but this functionality is already present in the code — delegate_to_scoped_klass checks if method name looks like /\A[a-zA-Z_]\w*[!?]?\z/ and if not, it executes different code in module_eval, which calls the method with send. It's just the def which is written improperly. So my patch isn't essentially adding any new feature, it's just fixing the broken code. If you don't want to support non-standard scope names, then probably this part of code should be removed.

[jeremy]

Imagine you didn't know about this pull request or issue, and you're reading this test. What does it communicate to you?

[jeremy]

@KrzysiekJ Agree this should work just on the basis of consistency. I wouldn't keep pushing it as a feature, though. If anything, we want to encourage using "Ruby method" naming for scopes. I made a couple comments on the patch. Thanks! 😁

[KrzysiekJ]

Thanks for the review. I've removed module_eval usage, added a comment to the test and amended those changes to the first commit.

[senny]

can you rebase your branch and add a CHANGELOG entry?

[KrzysiekJ]

Ok, I've added the entry and rebased branch.

[senny]

@KrzysiekJ great! let's wait for @jeremy to merge it.

[volochaev]

👎

[jeremy]

@KrzysiekJ No need for a changelog entry. Sorry about that! Prefer to treat this as a cleanup & refactoring, not to advertise it as a new feature.

The comment on the unit test is good, but the test code itself doesn't communicate what's going on. Rather than defining the scope elsewhere then calling it in the test, consider defining the scope within the test. Also, asserting that no exceptions are raised is imprecise: "nothing blows up!" Assert that the scoping returns what you expect it to.

[KrzysiekJ]

@jeremy No problem, I’ve removed the changelog entry, improved the test and updated the pull request.