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

Switch to pydantic's BaseSettings for the config file? #152

Closed
CasperWA opened this issue Feb 3, 2020 · 2 comments · Fixed by #226
Closed

Switch to pydantic's BaseSettings for the config file? #152

CasperWA opened this issue Feb 3, 2020 · 2 comments · Fixed by #226
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers priority/medium Issue or PR with a consensus of medium priority

Comments

@CasperWA
Copy link
Member

CasperWA commented Feb 3, 2020

Was there a reason we made our own config class rather than basing it off pydantic's BaseSettings

Originally posted by @shyamd in #134 (comment)

Link to pydantic documentation on the subject.

@CasperWA CasperWA added the question Further information is requested label Feb 3, 2020
@shyamd
Copy link
Contributor

shyamd commented Feb 4, 2020

Pros for switching to BaseSettings:

  • default arguments for settings built into the model
  • automatic validation of values built into the model
  • easy to add new values - Only need to modify in one place
  • Can modify settings with environment variables
  • Consistent with the rest of the pydantic based model infrastructure

Cons:

  • Basically lose the ini file as BaseSettings is designed for JSON files

@CasperWA
Copy link
Member Author

CasperWA commented Feb 5, 2020

I think this will be a good move - but is at the same time maybe medium priority at best? Losing the ini file is not a big issue for me, honestly. In my own implementation (based on this package) I use a JSON file.

@CasperWA CasperWA added enhancement New feature or request good first issue Good for newcomers priority/medium Issue or PR with a consensus of medium priority and removed question Further information is requested labels Feb 5, 2020
@CasperWA CasperWA added this to Needs discussion/thought in Road to optimade-python-tools 1.0 Mar 5, 2020
Road to optimade-python-tools 1.0 automation moved this from Needs discussion/thought to Done Mar 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers priority/medium Issue or PR with a consensus of medium priority
Development

Successfully merging a pull request may close this issue.

2 participants