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

Add support for custom default controller configuration #788

Merged
merged 7 commits into from
Mar 31, 2017

Conversation

codebycliff
Copy link
Collaborator

@codebycliff codebycliff commented Mar 30, 2017

Description

This pull request adds configuration to draper so that you can customize the default controller. According to the documentation in the README:

Configuration

Draper works out the box well, but also provides a hook for you to configure its
default functionality. For example, Draper assumes you have a base ApplicationController.
If your base controller is named something different (e.g. BaseController),
you can tell Draper to use it by adding the following to an initializer:

Draper.configure do |config|
 config.default_controller = BaseController
end

This also lays the ground work for other configuration options in the future. This work was originally inspired from the #776 by @nick-donald.

Todos

  • Tests
  • Documentation
  • Fix state leaking from tests

Related

Issue #776
Issue #573
Pull request #776

end

it 'defaults default_controller to ApplicationController' do
skip
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These tests all pass every time when I run just this file. However, these last two tests cause the entire suite to fail with an error about leaking a double. Due to it being a state leaking issue, it's not a consistent test that fails. The two different errors that keep seeing are:
image

and

image

I'm not sure how to go about fixing those as I'm not super familiar with the test setup on this project. I will continue to look into it, but could use some advice if someone has some.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was able to fix this issue. The problem was that one of the tests in the build_strategy_spec.rb was stubbing a constant that was overriding the new class definition in the spec_helper.rb. The specific test that was stubbing was able to be re-written without mocks now that the ApplicationController class is defined. I believe the way I re-wrote the test still tests for the same functionality. Somebody correct me if that's not the case. The commit is here:
a886d28

@codebycliff codebycliff added this to the 3.0.0 milestone Mar 30, 2017
@codebycliff
Copy link
Collaborator Author

This is ready for review now if anybody wants to take a look (e.g. @varyonic @seanlinsley @syguer). I added the new functionality to the dummy application as well to increase the code coverage.

@syguer
Copy link
Contributor

syguer commented Mar 31, 2017

Looks good! 👏

@syguer
Copy link
Contributor

syguer commented Mar 31, 2017

But I wonder why draper needs to depend on Controller layer... 🤔
Of course I know, we need a context of controller. I hope we can divide its dependency someday 😄

@codebycliff
Copy link
Collaborator Author

Yes, I agree. Maybe we can extract that dependency into optional functionality. The basic decorator pattern should be able to work without the need for a view context. But, baby steps for now..

@syguer
Copy link
Contributor

syguer commented Mar 31, 2017

👍

@codebycliff codebycliff merged commit ff36bd8 into master Mar 31, 2017
@codebycliff codebycliff deleted the feature/default-controller-configuration branch March 31, 2017 14:05
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.

2 participants