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

Load exhibit classes dynamically #1

Merged
merged 5 commits into from May 20, 2012
Merged

Conversation

mulderp
Copy link

@mulderp mulderp commented May 17, 2012

This one goes through the /app/exhibits directory and load the classes from there. As there is no /app/exhibits directory in the gem, I have no test yet (probably, an /app directory would be needed in the /test directory)

@codeodor
Copy link
Member

Thanks for this!

I like the idea behind f8d9fb3 (load instead of require for development) and I had not thought about doing that. But is there advantage to loading the files here instead of letting Rails do it?

For d2ee630 (dynamically load exhibits) instead of searching through the file system, I think we can use self.inherited(child) on Exhibit to track the exhibits.

This has a few benefits:

  1. it does not force people to have an exhibits folder in their app if they don't want it
  2. it does not prevent people from putting something related to their exhibits which might not be an exhibit itself inside the same folder
  3. it does not assume sticking to the convention of ClassName in class_name.rb (which they should, I suppose)
  4. it does not recalculate the list of exhibits on each call.

Thoughts?

@codeodor
Copy link
Member

Check out dd727ad to see what I was talking about with self.inherited ... Do you think that will have the same effect as what we're trying to achieve with dynamically keeping track of the exhibits?

@mulderp
Copy link
Author

mulderp commented May 17, 2012

Cool, that looks very elegant. Yes, just tried it, and Rails discovers the new exhibits and changes in them dynamically.

Additionally, it's indeed a benefit of not asking for a special folder setup when using exhibits. Regarding the naming of exhibits with _exhibit, it's maybe a question if we look at them more like a "controller" type of class or as part of a domain language like children of ActiveModels. Also, the naming of partials may come to ones mind, where the "" marks the special context in which partials are used. Anyway, it's something which can evolve over time. I would say, for now, the pull request can be closed.

@mulderp mulderp closed this May 17, 2012
@codeodor
Copy link
Member

Cool, thanks for the discussion and input!

@mulderp
Copy link
Author

mulderp commented May 18, 2012

Hi Sam,

eventually use of "inhereted" needs some more tweaking. Somehow, I missed yesterday, that "inherited" seems to be triggered only when the first instance of an Exhibit class is loaded. So, an application exhibit is not available until the first instance has been made. As workaround, I instantiate an exhibit object during startup of the application.

Maybe this example illustrates better what I mean. Without making an instance of an exhibit in the config/initializer, the class variable @@exhibits gives:

Registered exhibits: [Exhibit::Exhibited, EnumerableExhibit]

When making an instance ReadingListExhibit of an exhibit in config/niitializer, the registered exhibits are:

Registered exhibits: [Exhibit::Exhibited, EnumerableExhibit, ReadingListExhibit]

What do you think? Would it make sense to put some code that walks the exhibits folder during startup?

@mulderp
Copy link
Author

mulderp commented May 19, 2012

Hmm.. the name of the problem seems to be "eager loading" of classes, as discusses here:

@mulderp
Copy link
Author

mulderp commented May 19, 2012

@mulderp mulderp reopened this May 19, 2012
@mulderp
Copy link
Author

mulderp commented May 19, 2012

Ok, the exhibits are discovered now automatically by having in config/application.rb :

config.after_initialize do
  DisplayCase.find_definitions
end

@codeodor
Copy link
Member

Ok, I see what you're saying. Good catch.

One question though before I merge this:

Does Rails auto-reload these files in development mode since we required them, and Rails did not? (Referring to this quote from the StackOverflow link you provided above: "With this you'd load all files under "app/components" but then Rails would not reload these objects, as they were not required by Rails, but by your own gem.")

If not, then we need to figure out a way around it (maybe create a new issue), ... or if so, then nothing to worry about.

Thanks!

@mulderp
Copy link
Author

mulderp commented May 19, 2012

Hm.. did not check yet, but my guess is that 'load' as default should work fine. The code of find_definitions is actually from factory_girl. I am back on my laptop later tomorrow, just got into weekend offline mode :)

@codeodor
Copy link
Member

Understandable. I'll check it tomorrow or Monday if you don't get around to it. I'm trying to get off the computer for the rest of the day myself. =)

codeodor added a commit that referenced this pull request May 20, 2012
Load exhibit classes dynamically
@codeodor codeodor merged commit 8137870 into objects-on-rails:master May 20, 2012
@codeodor
Copy link
Member

Thanks! Now to confirm whether or not Rails with auto-reload the exhibits.

I did confirm the problem does not exist in production, so we can probably add a Railtie to do this automatically for development mode.

@mulderp
Copy link
Author

mulderp commented May 21, 2012

Yes, just to confirm, your fix based on the Railtie works great in development. Thanks!

avdi pushed a commit that referenced this pull request Oct 16, 2012
Add context to applicable_to?
codeodor pushed a commit that referenced this pull request Apr 4, 2013
Add test that ensures .exhibit works when passed a context that is an instance of an anonymous class
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

2 participants