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.

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

Fix regression with using acts_as_list on base classes #147

Merged
merged 1 commit into from Jan 14, 2015

Conversation

botandrose
Copy link
Contributor

#123 introduced a regression, since acts_as_list_class now returns the subclass, not the superclass. My use-case is as follows:

class Segment < AR::Base
  acts_as_list
end

class Video < Segment; end
class Quiz < Segment; end
class Exam < Segment; end

The application is an online course, which is composed of videos, quizzes, and exams. acts_as_list keeps these in order. However, after #123, lower_item will return the next item of the same subclass, skipping over other segments of other subclasses, which is not at all desired.

If one desires this behavior, declaring acts_as_list on the subclasses separately makes sense. This PR reverts the regression, and includes tests for both use-cases.

swanandp added a commit that referenced this pull request Jan 14, 2015
Fix regression with using acts_as_list on base classes
@swanandp swanandp merged commit 84325ed into brendon:master Jan 14, 2015
@swanandp
Copy link
Collaborator

@botandrose We want to support Rubinius as well, but the issue is about bundler and rbx versioning, so I'll let it slide for now.

@botandrose
Copy link
Contributor Author

Thank you for the quick response! <3

@severin
Copy link

severin commented Jan 16, 2015

Thanks for fixing this! Just ran into the same issue.

@@ -84,7 +84,7 @@ def acts_as_list_top
end

def acts_as_list_class
self.class
::#{self.name}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please explain what does this string mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure! There's a lot going on in this little change, can you be a little more specific with your question? Are you asking about the change from self.class to self.name, or are you asking about the syntax madness of ::#{}?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm asking about syntax madness.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yeah. So there are two things going on there.

One is that we're inside a class_eval metaprogramming block, so any string interpolation with #{} gets applied directly to the ruby code being constructed. So if the value of self.name is "TheBaseClass" when the acts_as_list macro is invoked, that line compiles to ::TheBaseClass.

Second is ::, which is the scope resolution operator. This is there to make sure that we are definitely getting the "root-level" TheBaseClass, instead of unintentionally getting something like SomeOtherModuleThatAlsoContains::TheBaseClass.

Does this help?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes this is very helpful! I've missed that it happens inside the string!
Thank you!

@brendon brendon mentioned this pull request Jul 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants