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

Fix some load errors related to different gem versions being available #184

Merged
merged 2 commits into from
Nov 29, 2024

Conversation

phil-janeapp
Copy link
Contributor

Context

The functionality introduced via #176 is great, but it has some subtle interactions with other libraries that are causing breaking bugs for us:

  • Depending on the version of psych loaded, using YAML.load_file directly won't work if the config file contains aliases
  • If MultiJson is present, JSON::Parser may not exist yet

Related tickets

What's inside

  • Ensure we utilize the wrapper for load_yaml to deal with aliases properly
  • Utilize JSON::Validator.parse as it handles MultiJson and other inconsistencies

Checklist:

  • I have added tests
    • Very difficult to add tests to this without moving to a multi-gemspec configuration. Not sure if that's worth it for this (although it might be)
  • I have made corresponding changes to the documentation

- Ensure we utilize the wrapper for `load_yaml` to deal with aliases properly
- Utilize `JSON::Validator.parse` as it handles MultiJson and other inconsistencies
Copy link
Owner

@bibendi bibendi left a comment

Choose a reason for hiding this comment

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

Hey Phil,
Sorry about that! Just one minor tweak, and I'll merge the changes.

lib/dip/config.rb Show resolved Hide resolved
@bibendi bibendi merged commit c527517 into bibendi:master Nov 29, 2024
6 checks passed
@bibendi
Copy link
Owner

bibendi commented Nov 29, 2024

Thank you!

@bibendi
Copy link
Owner

bibendi commented Nov 29, 2024

@oleander FYI

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