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

add file-based config #11 #26

Closed
wants to merge 1 commit into from
Closed

add file-based config #11 #26

wants to merge 1 commit into from

Conversation

jaymcgrath
Copy link
Contributor

Hi @JoranHonig - I'm opening this PR for discussion of my implementation of issue #11.

I agree that adding an unmaintained dependency would be bad. I've taken your suggestion and worked up this implementation of basic file-based config, using pyYAML's safe_load to load the yaml into a dict.

I'm using dict.get to avoid KeyErrors and return defaults that match the ones click returns.

Before I polish this up and add test coverage and catch exceptions around validating the yml file, I wanted to run this by you to see what you thought.

Thanks!

@jaymcgrath
Copy link
Contributor Author

Next steps:
Examine PyYAML to determine which exceptions it raises - wrap this in a try/catch
Format and include an example yml config file
Update docs
Add tests

Possible enhancements:
Currently, we assume that the file is in the current working directory. Maybe we should try to load it as an absolute path, then if that fails, prepend the cwd and try again?

@JoranHonig
Copy link
Owner

@jaymcgrath Thanks for the contribution! 🎉

I'm travelling for the first few days of this week so I might not get to review your comments & pr until thursday

@jaymcgrath jaymcgrath closed this Nov 1, 2021
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