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

Export improvements #660

Merged
merged 8 commits into from
Nov 28, 2023
Merged

Export improvements #660

merged 8 commits into from
Nov 28, 2023

Conversation

savente93
Copy link
Contributor

@savente93 savente93 commented Nov 24, 2023

Issue addressed

Fixes #657

Explanation

  • I ended up not allowing multiple instances of time tuples because that would end up either overwriting the previous exports, or changing the file names, which I didn't want to do.
  • Other changes are based on the discussion Hélène and I had.

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)

@savente93 savente93 marked this pull request as ready for review November 24, 2023 15:06
@savente93 savente93 requested a review from hboisgon November 24, 2023 15:06
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.

Almost there @savente93, thanks for looking into it!
I think the multiple -t options is actually implemented (it was support multiple sources which at the time were using -t instead of -s now :) )
What I miss is passing the extra arguments from config and for you to check if the config is properly parsed by your function still with the changes implemented

docs/changelog.rst Outdated Show resolved Hide resolved
docs/user_guide/data_overview.rst Outdated Show resolved Hide resolved
docs/user_guide/data_overview.rst Outdated Show resolved Hide resolved
docs/user_guide/data_overview.rst Outdated Show resolved Hide resolved
docs/user_guide/data_overview.rst Outdated Show resolved Hide resolved
hydromt/cli/main.py Outdated Show resolved Hide resolved
hydromt/cli/main.py Outdated Show resolved Hide resolved
hydromt/cli/main.py Outdated Show resolved Hide resolved
hydromt/cli/main.py Outdated Show resolved Hide resolved
hydromt/cli/main.py Show resolved Hide resolved
@savente93 savente93 requested a review from hboisgon November 28, 2023 08:31
@savente93
Copy link
Contributor Author

What I miss is passing the extra arguments from config and for you to check if the config is properly parsed by your function still with the changes implemented

Sorry I don't understand what you are asking here, could you rephrase it for me? other than that, all of the comments should be resolved now.

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.

Looks all good :) I tried to explain better what I meant but your solution works too.

hydromt/cli/main.py Show resolved Hide resolved
@savente93 savente93 merged commit 3285054 into main Nov 28, 2023
8 checks passed
@savente93 savente93 deleted the export-improvements branch November 28, 2023 09:08
@savente93 savente93 mentioned this pull request Jan 9, 2024
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.

Improve export CLI
2 participants