478 caravan forcing downloads and unzips shapefiles for all caravan with each basin leads to errors fixes #478

[RolfHut]

allowed the option in shape to point to combined.shp file that is already downloaded. Removed the check on polygons, which threw an error that was not correct stopping workflows

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 8 lines in your changes missing coverage. Please review.

Project coverage is 78.32%. Comparing base (ce00996) to head (ca884ee).

Files with missing lines	Patch %	Lines
src/ewatercycle/_forcings/caravan.py	0.00%	7 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #479      +/-   ##
==========================================
- Coverage   78.49%   78.32%   -0.17%     
==========================================
  Files          28       28              
  Lines        1888     1892       +4     
  Branches      162      163       +1     
==========================================
  Hits         1482     1482              
- Misses        347      352       +5     
+ Partials       59       58       -1     

Files with missing lines	Coverage Δ
src/ewatercycle/_forcings/caravan.py	79.79% <0.00%> (-3.36%)	⬇️

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[MarkMelotto]

Do we want to make some tests for this?
As it should be done and can easily be implemented!

[RolfHut]

we absolutely want to write some tests for this.

I think we should have at least one model run test (HBVLocal?)

[MarkMelotto]

@RolfHut we can do that.
Or just basic known shapefiles and other things that are not exactly shapefiles ;)

[BSchilperoort]

This would not be the most robust way of checking for an already existing combined shapefile. It's also a bit difficult to document.

Maybe it would be better to;

• Not download automatically anymore if shape=None, although this breaks existing code using this feature
• Add a keyword argument (like the existing basin_id one) for downloading the shapes, download_shape which defaults to false.

# get download_shape from kwargs, default to false;
download_shape = kwargs.get("download_shape", False) 

• Add another kwarg combined_shapefile that you can use to point to the combined shapefile with all basins

combined_shape = kwargs.get("combined_shape")
if combined_shape is not None:
    combined_shape = Path(combined_shape)
    if not combined_shape.exists():
        msg = "Could not find combined shapefile."
        raise FileNotFoundError(msg)

These kwargs can be properly documented in the docstring, and validating the user input is a bit easier this way.

[BSchilperoort]

Do we want to make some tests for this?

Just be aware that downloading data from the internet during testing is not very desirable. You can have some test data included in the repository however, and "mock" the download of the test so it just points to that file. See https://docs.pytest.org/en/stable/how-to/monkeypatch.html

I think we should have at least one model run test (HBVLocal?)

I think this would be good, but the tests should also be able to run without having a model installed.
You can do this by only running these tests if you can import the package;

def model_available()
    try:
        import ewatercycle_leakybucket
        return True
    except ModuleNotFoundError:
        return False

MODEL_AVAILABLE = model_available()

@pytest.mark.skipif(not MODEL_AVAILABLE, reason="requires leakybucket to be installed")
def test_function(): ...

See https://docs.pytest.org/en/stable/how-to/skipping.html#id1