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

Replace kaptan with direct PyYAML SafeLoader usage #71

Closed
wants to merge 1 commit into from

Conversation

rjocoleman
Copy link
Contributor

@rjocoleman rjocoleman commented Apr 3, 2023

This PR replaces the kaptan configuration parser with direct usage of PyYAML's SafeLoader class.

Why:

  • Kaptan depends on PyYAML>=3.13,<6, which is lower than the currently released 6.0. This can lead to dependency resolution issues.
  • Kaptan is pretty much unmaintained with the last release to pypi in 2019 and the only developer who had been committing was unable to get access and deprecated it in their own projects: Admin privileges? emre/kaptan#176

How:

Caveats:

  • Only .yaml and yml file extensions are supported. This check can be easily removed to revert to previous filename behaviour.
  • Technically the previous usage of Kaptan allowed yaml, json, pyfile, dict and ini handlers. This was not tested nor documented. If is an actual feature and needs to be maintained then this code will not suffice, and a more feature config reader would be needed (with many more tests!).
  • I was not able to get all the tests to pass in my local for master branch, the same tests fail for this branch for me. I don't they are related to my changes.

Copy link

@legalsylvain legalsylvain left a comment

Choose a reason for hiding this comment

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

@sbidoul could you run test ? (As it is a first contribution)

Copy link
Contributor

@ap-wtioit ap-wtioit left a comment

Choose a reason for hiding this comment

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

Tested locally in doodba with python 3.10 and this fixes #72

@sbidoul
Copy link
Member

sbidoul commented Jul 22, 2023

Thanks for this contrib. Since there was a merge conflict I finished it in #76.

@sbidoul sbidoul closed this Jul 22, 2023
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.

5 participants