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

Don't include BreadcrumbsOnRails into the ActionController::API #99

Merged
merged 4 commits into from Aug 11, 2016

Conversation

Projects
None yet
4 participants
@jcoyne
Contributor

jcoyne commented Aug 9, 2016

Alternative proposal combining a test with the fix from #77

@jcoyne

This comment has been minimized.

Show comment
Hide comment
@jcoyne

jcoyne Aug 9, 2016

Contributor

It looks like this fix isn't going to work on ruby 2.0.x. because include is private there. Can you merge #95 first and then I can rebase this?

Contributor

jcoyne commented Aug 9, 2016

It looks like this fix isn't going to work on ruby 2.0.x. because include is private there. Can you merge #95 first and then I can rebase this?

@weppos

This comment has been minimized.

Show comment
Hide comment
@weppos

weppos Aug 9, 2016

Owner

Removing support for Ruby and/or Rails versions requires a major bump, as I want to be consistent with SemVer.

I propose to use send(:include) that will bypass the limit for this version, so that I can package a 3.0.1.

After that, I'm fine to merge #95 and drop support for previous versions. Please note that #95 also requires an update to the README and .gemspec

Owner

weppos commented Aug 9, 2016

Removing support for Ruby and/or Rails versions requires a major bump, as I want to be consistent with SemVer.

I propose to use send(:include) that will bypass the limit for this version, so that I can package a 3.0.1.

After that, I'm fine to merge #95 and drop support for previous versions. Please note that #95 also requires an update to the README and .gemspec

@jcoyne

This comment has been minimized.

Show comment
Hide comment
@jcoyne

jcoyne Aug 9, 2016

Contributor

I've added send :include, ...

Contributor

jcoyne commented Aug 9, 2016

I've added send :include, ...

@jcoyne

This comment has been minimized.

Show comment
Hide comment
@jcoyne

jcoyne Aug 9, 2016

Contributor

I wonder if this will hit the Cannot define multiple 'included' blocks for a Concern in production mode?

Contributor

jcoyne commented Aug 9, 2016

I wonder if this will hit the Cannot define multiple 'included' blocks for a Concern in production mode?

@jcoyne

This comment has been minimized.

Show comment
Hide comment
@jcoyne

jcoyne Aug 9, 2016

Contributor

It doesn't seem to be a problem. I tested this change in production mode.

Contributor

jcoyne commented Aug 9, 2016

It doesn't seem to be a problem. I tested this change in production mode.

Show outdated Hide outdated lib/breadcrumbs_on_rails/railtie.rb
@@ -10,7 +10,7 @@ module BreadcrumbsOnRails
class Railtie < Rails::Railtie
ActiveSupport.on_load(:action_controller) do
include BreadcrumbsOnRails::ActionController
::ActionController::Base.send :include, BreadcrumbsOnRails::ActionController

This comment has been minimized.

@jonathansimmons

jonathansimmons Aug 10, 2016

This is nit picky but I think most would agree we should use parenthesis here: https://github.com/bbatsov/ruby-style-guide#method-invocation-parens

@jonathansimmons

jonathansimmons Aug 10, 2016

This is nit picky but I think most would agree we should use parenthesis here: https://github.com/bbatsov/ruby-style-guide#method-invocation-parens

@jcoyne

This comment has been minimized.

Show comment
Hide comment
@jcoyne

jcoyne Aug 10, 2016

Contributor

Added parens

Contributor

jcoyne commented Aug 10, 2016

Added parens

@weppos weppos self-assigned this Aug 11, 2016

@weppos weppos added the bug label Aug 11, 2016

@weppos weppos merged commit 3a3ac0c into weppos:master Aug 11, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

weppos added a commit that referenced this pull request Aug 11, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment