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

Support Psych 4.x gem #10

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Support Psych 4.x gem #10

wants to merge 2 commits into from

Conversation

AdamBukowczyk
Copy link

@AdamBukowczyk AdamBukowczyk commented Nov 20, 2023

Ruby 3.1 comes with Psych 4, which has a major breaking change
The new YAML loading methods (Psych 4) do not load aliases unless they get the aliases: true argument.
The old YAML loading methods (Psych 3) do not support the aliases keyword.

Related PR:

Ruby 3.1 comes with Psych 4, which has a major breaking change
The new YAML loading methods (Psych 4) do not load aliases unless they get the aliases: true argument.
The old YAML loading methods (Psych 3) do not support the aliases keyword.
@AdamBukowczyk AdamBukowczyk requested review from wojtowicz and removed request for wojtowicz November 21, 2023 05:20
@@ -14,7 +14,7 @@ class Engine < Rails::Engine

private
def apply_yaml_config(yaml)
cfg = (YAML.load(ERB.new(yaml).result)||{}).fetch(Rails.env, {})
cfg = (YAML.load(ERB.new(yaml).result, aliases: true)||{}).fetch(Rails.env, {})

Choose a reason for hiding this comment

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

Ok so according to description this aliases: true is something new in ruby 3.1 correct? Then this changes requires ruby version > 3.1?

So looks like we have to add this dependency to our gemspec similar like we have in other gems (example)

in our case

 s.required_ruby_version = '>= 3.1.0'

Choose a reason for hiding this comment

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

Looks like this gem is only used in CAS application so this is good. Just please double check and confirm :)

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