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

Allow [[core.data.variable]] to be split across files #710

Merged
merged 4 commits into from
Jan 31, 2025

Conversation

davidorme
Copy link
Collaborator

@davidorme davidorme commented Jan 30, 2025

Description

This PR updates config_merge to handle the special case where configuration sources provide identical key paths but the values are both lists. In this case, the config merges the two lists, rather than adding a conflict.

The main (only?) structure giving this is in TOML is the array of tables, which is what we use for [[core.data.variable]]. This PR doesn't guarantee that the merged lists make any sense but downstream configuration checkers should handle that. It also means you could in split other array of tables structures (like [[plants.pft_type]]) across files. That strikes me as stupid, but not wrong, and I think there are clear advantages of allowing model-specific data to be bundled with model config settings.

Fixes #709
Fixes #234

Type of change

  • New feature (non-breaking change which adds functionality)
  • Optimization (back-end change that speeds up the code)
  • Bug fix (non-breaking change which fixes an issue)

Key checklist

  • Make sure you've run the pre-commit checks: $ pre-commit run -a
  • All tests pass: $ poetry run pytest

Further checks

  • Code is commented, particularly in hard-to-understand areas
  • Tests added that prove fix is effective or that feature works
  • Relevant documentation reviewed and updated

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.72%. Comparing base (97ee94b) to head (e8d197c).

Additional details and impacted files
@@           Coverage Diff            @@
##           develop     #710   +/-   ##
========================================
  Coverage    94.72%   94.72%           
========================================
  Files           73       73           
  Lines         4930     4932    +2     
========================================
+ Hits          4670     4672    +2     
  Misses         260      260           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@jacobcook1995 jacobcook1995 left a comment

Choose a reason for hiding this comment

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

Looks sensible to me! I think it might be worth adding a separate plants config toml file as part of this PR so that the test_ve_run test will confirm that the change is working for the intended use case

@davidorme
Copy link
Collaborator Author

I think it might be worth making a separate plants config as part of this PR so that the test_ve_run test will confirm that the change is working for the intended use case

That's basically coming in the plants model rejig in #707 anyway, so I'll merge here.

@davidorme davidorme merged commit 4cab25c into develop Jan 31, 2025
13 checks passed
@davidorme davidorme deleted the 709-allow-data-to-be-configured-in-different-files branch January 31, 2025 09:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants