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

Call Module#module_parent instead of deprecated #parent #349

Closed
wants to merge 2 commits into from
Closed

Call Module#module_parent instead of deprecated #parent #349

wants to merge 2 commits into from

Conversation

tommy-gilligan
Copy link
Contributor

To silence the deprecation warning mentioned at #346

So that it is backwards compatible with Rails versions < 6:

  • call module_parent if ActiveSupport version >= 6.0.0.beta1
  • otherwise call parent

@joevandyk
Copy link
Contributor

anything preventing this from being merged/released?

Copy link
Contributor

@kbrock kbrock left a comment

Choose a reason for hiding this comment

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

looks nice.

not sure if there are any 6.0 gemfiles to test, I'll try and add that if my first attempt at travis is passing.

@@ -32,7 +32,12 @@ def initialize(model_class, options)
end

def hierarchy_class_for_model
hierarchy_class = model_class.parent.const_set(short_hierarchy_class_name, Class.new(ActiveRecord::Base))
parent_class = if ActiveSupport.version >= Gem::Version.new('6.0.0.beta1')
Copy link
Contributor

@kbrock kbrock Sep 10, 2019

Choose a reason for hiding this comment

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

may want to to_s for the comparison - it simplifies things

Also, rails 6.0 has been released, so probably want to fix that too

Suggested change
parent_class = if ActiveSupport.version >= Gem::Version.new('6.0.0.beta1')
parent_class = if ActiveSupport.version.to_s >= "6.0.0"

@@ -32,7 +32,12 @@ def initialize(model_class, options)
end

def hierarchy_class_for_model
hierarchy_class = model_class.parent.const_set(short_hierarchy_class_name, Class.new(ActiveRecord::Base))
parent_class = if ActiveSupport.version >= Gem::Version.new('6.0.0.beta1')
Copy link
Contributor

Choose a reason for hiding this comment

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

Could also be

Suggested change
parent_class = if ActiveSupport.version >= Gem::Version.new('6.0.0.beta1')
parent_class = if ActiveSupport::VERSION::MAJOR >= 6

Copy link
Contributor

@kbrock kbrock left a comment

Choose a reason for hiding this comment

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

Just noticed this changed the gemfiles directly instead of changing the Appraisal file

@@ -7,7 +7,7 @@ gem "activerecord", "~> 4.2.0"
platforms :ruby do
gem "mysql2", "< 0.5"
gem "pg", "~> 0.21"
gem "sqlite3"
gem "sqlite3", "~> 1.3", "< 1.4"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think these gemfiles have been fixed at their source in appraisals.

Could you remove the changes to the gemfiles from this PR and ensure that the tests work for you?

@@ -7,7 +7,7 @@ gem "activerecord", "~> 5.0.0"
platforms :ruby do
gem "mysql2"
gem "pg"
gem "sqlite3"
gem "sqlite3", "~> 1.3", "< 1.4"
Copy link
Contributor

Choose a reason for hiding this comment

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

yea, remove this one too

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.

4 participants