search

Deltares / hydromt

HydroMT: Automated and reproducible model building and analysis
https://deltares.github.io/hydromt/
MIT License
65 stars 28 forks source link

Cli integration tests #988

Closed savente93 closed 2 weeks ago

savente93 commented 3 weeks ago

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_dict or from_yml function 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

savente93 commented 2 weeks ago

Most of the sonar clouds errors are introduced by touching code that isn't actually in the scope of this PR (i.e. refactoring export_data) so I think it's fine to leave it as is.

Jaapel commented 2 weeks ago

Also SonarCloud makes a good point, would correct that one.

savente93 commented 2 weeks ago

You mean export_data?

savente93 commented 2 weeks ago

because that would mean basically fixing #886 as well in this pr

Jaapel commented 2 weeks ago

because that would mean basically fixing #886 as well in this pr

Yeah no need, just making sure I understand the changes

savente93 commented 2 weeks ago

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

savente93 commented 2 weeks ago

Override test now works again, with one small addition. I couldn't figure out why the convention resolver couldn't find the vito file for the re class in the example, so I just edited that bit out. I'll have to add that bit back in at some point, but I don't think it's necessary for this PR, and belongs more in a PR to update the examples.

Jaapel commented 2 weeks ago

In ModelRoot and in multiple functions in cli.main a FileHandler for logging is created, pointing to the same file. I took a few stabs at it, but I think solving it would require redoing logging in HydroMT, just relying on the logging hierarchy in the builtin logging module, instead of passing loggers around manually.