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

Require ActiveSupport #298

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

benhutton
Copy link

@benhutton benhutton commented Jan 31, 2022

This fixes #297.

@benhutton benhutton marked this pull request as ready for review January 31, 2022 18:45
Copy link

@apotheon apotheon left a comment

Choose a reason for hiding this comment

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

I tested this change with a gem that requires carmen, and it works flawlessly. My only question is whether require 'active_support/core_ext/string' is still needed. It does not appear necessary at all.

This change, in and of itself, appears to fix the issue in question, though. I approve it. If anyone is unsure about the need for the other require line, and wants to investigate further, I see no reason to hold up this fix in the mean time.

@benhutton
Copy link
Author

@apotheon it looks like the other require line came in through 5d456f5. Some of that code is still in use today.

require 'active_support' doesn't actually require any of the libraries inside of AS. You still need to do that individually. require 'active_support' is basically setup code. See https://guides.rubyonrails.org/active_support_core_extensions.html#stand-alone-active-support for more details!

@apotheon
Copy link

Thanks. I stand corrected regarding the other require.

@apotheon
Copy link

apotheon commented Mar 4, 2022

@benhutton - How do you feel about maintaining a fork and pulling in code from pull requests already submitted as needed? Perhaps name it "SanDiego" and publish it as a gem. That's assuming you know the codebase well enough and have time to be a maintainer.

@benhutton
Copy link
Author

@apotheon I definitely don't know this codebase at all! Sorry

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.

Error on require: uninitialized constant ActiveSupport::XmlMini::IsolatedExecutionState (NameError)
2 participants