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 support for configure using TOML files #444

Merged
merged 11 commits into from
Aug 3, 2023
Merged

Add support for configure using TOML files #444

merged 11 commits into from
Aug 3, 2023

Conversation

savente93
Copy link
Contributor

@savente93 savente93 commented Jul 14, 2023

Issue addressed

Fixes #376

Explanation

Very simple implementation, because almost everything is done using dictionaries all that was needed was just swap the yaml functionality for the toml. (doc updates to come)

Checklist

  • Updated tests or added new tests
  • Branch is up to date with main
  • Tests & pre-commit hooks pass
  • Updated documentation if needed
  • Updated changelog.rst if needed

Additional Notes (optional)

Add any additional notes or information that may be helpful.

@savente93
Copy link
Contributor Author

Since Toml doesn't support any type of None value, I also took the liberty to Fix #152

@savente93 savente93 linked an issue Jul 14, 2023 that may be closed by this pull request
@savente93 savente93 marked this pull request as ready for review July 14, 2023 16:41
@DirkEilander
Copy link
Contributor

Thanks for this this PR @savente93! I noticed that the PR does more than just fixing #376, which requests toml support for model configuration files in the Model.read_config / write_config methods. This PR also adds toml support for HydroMT data catalog and configuration files. We should have a discussion with the team/users if this is indeed wanted.

My initial thoughts:
In general I can imagine that different file formats will only confuse users.
For the HydroMT configuration file I see a benefit though. If the model configuration is in toml (like for most PMT HYD models) it might actually be convenient for users if the HydroMT configuration can also be done in toml.
For the HydroMT data catalog file I think we should stick to toml as it has many more "layers" for which I think toml is not very readable. Let's discuss at the next stand-up how to proceed with this.

What do you think @hboisgon @alimeshgi @B-Dalmijn @Tjalling-dejong ?

@savente93
Copy link
Contributor Author

right, that's true. The part of the catalog being in TOML was from when I still misunderstood the objective. I still think it's a nice feature though, especially given the recommendations from DSC. Do note that I'm not suggesting we move everything to toml, this is simply support for it, so it would enable users to have all their config files in toml if they'd like. But as you say, let's indeed discuss in the next standup.

@alimeshgi
Copy link
Contributor

I agree that using different file formats can potentially confuse users. Consistency in file formats often helps maintain simplicity and ease of use. Yes, let's discuss it in the next stand-up.

@savente93 savente93 marked this pull request as draft July 25, 2023 08:40
@savente93
Copy link
Contributor Author

Converted to draft until we can reach a consensus on #452

@savente93 savente93 marked this pull request as ready for review August 2, 2023 09:13
Copy link
Contributor

@DirkEilander DirkEilander left a comment

Choose a reason for hiding this comment

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

LGTM!

@savente93 savente93 merged commit 7a78837 into main Aug 3, 2023
8 checks passed
@savente93 savente93 deleted the toml-support branch August 3, 2023 16:37
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.

ENH: Support toml format in config.configread Kwargs with None need to be passed using 'null' in yml files
3 participants