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

Implement default file path pattern for tables component #869

Merged
merged 3 commits into from
Apr 5, 2024

Conversation

savente93
Copy link
Contributor

@savente93 savente93 commented Apr 4, 2024

Issue addressed

Fixes #864

Explanation

In today's refinement we decided that components should take some variation of default_filename at init, and store that so it can be used for reading and writing unless the user overrides it by passing in an argument of their own.

In addition we did not explicitly discuss today, but it seems we have reached a consensus that:

  • components that can contain multiple objects (e.g. they store a dictionary of something) should be names in plural form
  • We do not want to expose constants for our default paths.

This PR brings TablesComponent up to date with that convention. I don't think any of the changelogs or documentation has to be updated for this, but feel free to let me know if you disagree.

Checklist

  • Updated tests or added new tests
  • Branch is up to date with v1
  • Tests & pre-commit hooks pass
  • Updated documentation if needed
  • Updated changelog.rst if needed
  • For predefined catalogs: update the catalog version in the file itself, the references in data/predefined_catalogs.yml, and the changelog in data/changelog.rst

Additional Notes (optional)

Add any additional notes or information that may be helpful.

@savente93 savente93 marked this pull request as ready for review April 4, 2024 12:38
@savente93 savente93 requested a review from hboisgon April 4, 2024 12:38
Copy link
Contributor

@hboisgon hboisgon left a comment

Choose a reason for hiding this comment

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

Thanks for the update! GridComponent should be also be updated by can be done in another PR. Before merging do we use default_filename or filename as the argument of the init function? I don't mind but then ConfigComponent should be updated.
In a way filename also sounds good and avoids for the user to give the filename in read and write all the time from config so default_filename makes sense for plugins developpers but not necessarily for users:

global:
  components:
    config:
      type: ConfigComponent
      filename: model_config.toml
    grid:
      type: GridComponent
      filename: grid.nc
steps:
  - config.create
  - grid.add_data_from_rasterdataset
  - write

hydromt/components/tables.py Outdated Show resolved Hide resolved
@savente93 savente93 merged commit b0a5308 into v1 Apr 5, 2024
8 checks passed
@savente93 savente93 deleted the tables-default-paths branch April 5, 2024 07:41
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