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

Fixing tests for ruamel yaml 0.18+ #199

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

Conversation

lilyinstarlight
Copy link

Fixes: #198

ruamel.yaml 0.18.0 removed deprecated API methods like safe_dump, safe_load_all, and usage of dump without a stream argument. This PR updates the usage by utilizing the yml object in pykwalify.compat for equivalent safe API methods

@Grokzen
Copy link
Owner

Grokzen commented Jan 22, 2024

@lilyinstarlight My gut feeling tells me that this PR probably should also include an update to the requirements file to pin ruamel.yaml>=0.18 then? Specially if this is probably not backwards compatible if they have changed around the functionality with the methods that ruamel uses?

@Grokzen
Copy link
Owner

Grokzen commented Jan 22, 2024

I am thinking that this big of an update to dependencies probably should bump up the release to 1.9.0 for instance and just say that either you upgrade rumale, or you stay behind until you can update ruamel wherever that is used

@lilyinstarlight
Copy link
Author

This is not a breaking change. The deprecated methods were only used in the tests (the rest of the usages were updated in 68405ed) and the functions used in this PR have been available since well before the currently required ruamel.yaml 0.16.0 for pykwalify

I did, however, just double check to make sure and the tests ran just fine with ruamel.yaml 0.16.0 locally as well

@lilyinstarlight
Copy link
Author

Thank you for the quick reply btw!

I can still bump the version if you would prefer, but I just figured I'd note this is a completely backwards compatible change (as far as I can tell), since I neglected to mention that in the initial description :)

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.

Tests fail with ruamel.yaml 0.18.0
2 participants