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 Rails 7.0 & Rails 7.1 #13

Merged
merged 1 commit into from
Dec 1, 2023

Conversation

mkhairi
Copy link
Contributor

@mkhairi mkhairi commented Aug 22, 2023

No description provided.

@mkhairi mkhairi changed the title Add Support for Rails 7 Add Support for Rails 7.0 & Rails 7.1 Oct 27, 2023
@mkhairi
Copy link
Contributor Author

mkhairi commented Nov 23, 2023

Hi @marshall-lee, Hope you're well. Could you kindly review this PR when you have a moment? Thanks.

@marshall-lee
Copy link
Owner

Hi @mkhairi please fix the CI.

@marshall-lee
Copy link
Owner

@mkhairi Lets drop Ruby 2.5 support, it's way too old.

@marshall-lee marshall-lee self-requested a review November 23, 2023 16:05
Comment on lines 5 to 21
def self.constantize(type_name)
# TODO: Remove this when we drop support for Rails < 7.0
if ActiveSupport::Dependencies.respond_to?(:constantize)
ActiveSupport::Dependencies.constantize(type_name)
else
type_name.constantize
end
end

def self.safe_constantize(type_name)
# TODO: Remove this when we drop support for Rails < 7.0
if ActiveSupport::Dependencies.respond_to?(:safe_constantize)
ActiveSupport::Dependencies.safe_constantize(type_name)
else
type_name.safe_constantize
end
end
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
def self.constantize(type_name)
# TODO: Remove this when we drop support for Rails < 7.0
if ActiveSupport::Dependencies.respond_to?(:constantize)
ActiveSupport::Dependencies.constantize(type_name)
else
type_name.constantize
end
end
def self.safe_constantize(type_name)
# TODO: Remove this when we drop support for Rails < 7.0
if ActiveSupport::Dependencies.respond_to?(:safe_constantize)
ActiveSupport::Dependencies.safe_constantize(type_name)
else
type_name.safe_constantize
end
end
# TODO: Remove this when we drop support for Rails < 7.0
if ActiveSupport::VERSION::MAJOR < 7
def self.constantize(type_name)
ActiveSupport::Dependencies.constantize(type_name)
end
def self.safe_constantize(type_name)
ActiveSupport::Dependencies.safe_constantize(type_name)
end
else
def self.constantize(type_name)
type_name.constantize
end
def self.safe_constantize(type_name)
type_name.safe_constantize
end
end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@marshall-lee, Opps.. Seems like it works without any rails version condition here. 😅

Copy link
Owner

Choose a reason for hiding this comment

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

@mkhairi Yes it works but ActiveSupport::Dependencies.constantize on older Rails leverages the ActiveSupport::Dependencies::ClassCache. We should do it too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mkhairi Yes it works but ActiveSupport::Dependencies.constantize on older Rails leverages the ActiveSupport::Dependencies::ClassCache. We should do it too.

@marshall-lee, Apologies, I'm unaware of about this. I have reinstated the Rails version condition based on the previous suggestion.

Copy link
Owner

Choose a reason for hiding this comment

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

@mkhairi Sure, please squash the commits

- Fixed constantize method to make compatible with rails 7
- Drop support for ruby 2.5
@marshall-lee marshall-lee merged commit faf2c2a into marshall-lee:master Dec 1, 2023
20 checks passed
@mkhairi mkhairi deleted the rails7 branch December 5, 2023 07:38
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