Cli integration tests #988
Conversation
| "-s", | ||
| "hydro_lakes", | ||
| "-r", | ||
| "{'subbasin': [-7.24, 62.09], 'uparea': 50}", |
There was a problem hiding this comment.
How would we export for a certain region? hydromt export should still be tested right?
There was a problem hiding this comment.
The reason (as far as I understand) that the region got removed from the cli, is that the way we construct regions has become much more complicated and interwoven with the components. This means that regions can no longer be specified independent of what type of components you have. The export can be used without a region and then all the data will be exported. aside from that we also have a file format for specifying what data needs to get exported. I'm not sure if this is updated yet, but regions will have to be specified there. There should be at least one test that uses the file format.
|
Most of the sonar clouds errors are introduced by touching code that isn't actually in the scope of this PR (i.e. refactoring |
| @@ -1006,10 +1006,11 @@ def export_data( | |||
| self, | |||
| data_root: Union[Path, str], | |||
| bbox: Optional[Bbox] = None, | |||
| time_tuple: Optional[TimeRange] = None, | |||
| time_range: Optional[TimeRange] = None, | |||
There was a problem hiding this comment.
Seems like you partially updated the export_data function to v1, but not fully?
There was a problem hiding this comment.
Yeah, that's true, these are some left over changes from before I decided to leave the export_data function alone, I can remove them if you like
| "--opt", | ||
| "setup_grid.res=0.05", | ||
| "-vv", | ||
| ] | ||
| _ = CliRunner().invoke(hydromt_cli, cmd) | ||
|
|
||
|
|
||
| @pytest.mark.skip( |
There was a problem hiding this comment.
Why are the override tests skipped?
There was a problem hiding this comment.
I'd have to write another yaml file for that since we don't have a way to specify model components from the CLI. I can do that if you like
There was a problem hiding this comment.
Does the same --fo flag exist now?
|
Also SonarCloud makes a good point, would correct that one. |
|
You mean |
|
because that would mean basically fixing #886 as well in this pr |
Yeah no need, just making sure I understand the changes |
|
There are still one or two issues that I can't figure out yet, I'll come back to those in an hour or so, I think I need to look at something else for a bit to recover my productivity |
|
Override test now works again, with one small addition. I couldn't figure out why the convention resolver couldn't find the |
|
In |
Issue addressed
Fixes #943
Explanation
I've decided to leave most of the cli code as is even though we wanted to reduce it, but the code for it works again. I've removed the tests that were purely about region arguments since we've decided to remove those. I haven't actually updated the code there yet, but I'll pick that up in #966 when the time comes, since I think that is closely related.
I also can't quite remember why I thought that I'd needed a
from_dictorfrom_ymlfunction for this one, but given that I've written it and we plan to add it later I think we might as well just add it. I'll do more testing and integration in the ticket mentioned above.General Checklist
main