Merge pull request #1018 from Ouranosinc/fix-963

History attribute shows args as kwargs

HISTORY.rst

@@ -14,6 +14,10 @@ Breaking changes
 ^^^^^^^^^^^^^^^^
 * The version pin for `bottleneck` (<1.4) has been lifted. (:pull:`1013`).
 * `packaging` has been removed from the `xclim` run dependencies. (:pull:`1013`).
+* The "history" string attribute added by xclim has been modified for readability: (:issue:`963`, :pull:`1018`).
+    - The trailing dot (``.``) was dropped.
+    - ``None`` inputs are now printed as "None" (and not "<NoneType>").
+    - Arguments are now always shown as keyword-arguments. This mostly impacts ``sdba`` functions, as it was already the case for ``Indicators``.
 
 Internal changes
 ^^^^^^^^^^^^^^^^

xclim/core/formatting.py

@@ -9,7 +9,7 @@
 import string
 from ast import literal_eval
 from fnmatch import fnmatch
-from inspect import _empty  # noqa
+from inspect import _empty, signature  # noqa
 from typing import Any, Dict, List, Mapping, Optional, Sequence, Union
 
 import xarray as xr
@@ -319,14 +319,16 @@ def update_history(
     )
     if len(merged_history) > 0 and not merged_history.endswith("\n"):
         merged_history += "\n"
-    merged_history += f"[{dt.datetime.now():%Y-%m-%d %H:%M:%S}] {new_name or ''}: {hist_str} - xclim version: {__version__}."
+    merged_history += f"[{dt.datetime.now():%Y-%m-%d %H:%M:%S}] {new_name or ''}: {hist_str} - xclim version: {__version__}"
     return merged_history
 
 
 def update_xclim_history(func):
     """Decorator that auto-generates and fills the history attribute.
 
     The history is generated from the signature of the function and added to the first output.
+    Because of a limitation of the `boltons` wrapper, all arguments passed to the wrapped function
+    will be printed as keyword arguments.
     """
 
     @wraps(func)
@@ -349,8 +351,11 @@ def _call_and_add_history(*args, **kwargs):
             name: arg for name, arg in kwargs.items() if isinstance(arg, xr.DataArray)
         }
 
+        # The wrapper hides how the user passed the arguments (positional or keyword)
+        # Instead of having it all position, we have it all keyword-like for explicitness.
+        bound_args = signature(func).bind(*args, **kwargs)
         attr = update_history(
-            gen_call_string(func.__name__, *args, **kwargs),
+            gen_call_string(func.__name__, **bound_args.arguments),
             *da_list,
             new_name=out.name,
             **da_dict,
@@ -364,8 +369,9 @@ def _call_and_add_history(*args, **kwargs):
 def gen_call_string(funcname: str, *args, **kwargs):
     """Generate a signature string for use in the history attribute.
 
-    DataArrays and Dataset are replaced with their name, floats, ints and strings are
-    printed directly, all other objects have their type printed between < >.
+    DataArrays and Dataset are replaced with their name, while Nones, floats,
+    ints and strings are printed directly.
+    All other objects have their type printed between < >.
 
     Arguments given through positional arguments are printed positionnally and those
     given through keywords are printed prefixed by their name.
@@ -388,7 +394,7 @@ def gen_call_string(funcname: str, *args, **kwargs):
     for name, val in chain:
         if isinstance(val, xr.DataArray):
             rep = val.name or "<array>"
-        elif isinstance(val, (int, float, str, bool)):
+        elif isinstance(val, (int, float, str, bool)) or val is None:
             rep = repr(val)
         else:
             rep = f"<{type(val).__name__}>"

xclim/testing/tests/test_formatting.py

@@ -1,3 +1,7 @@
+import datetime as dt
+import re
+
+from xclim import __version__
 from xclim.core import formatting as fmt
 from xclim.indicators.atmos import degree_days_exceedance_date, heat_wave_frequency
 
@@ -33,3 +37,23 @@ def test_indicator_docstring():
 
     doc = degree_days_exceedance_date.__doc__.split("\n")
     assert doc[20] == "  Default : >. "
+
+
+def test_update_xclim_history(atmosds):
+    @fmt.update_xclim_history
+    def func(da, arg1, arg2=None, arg3=None):
+        return da
+
+    out = func(atmosds.tas, 1, arg2=[1, 2], arg3=None)
+
+    matches = re.match(
+        r"\[([0-9-:\s]*)\]\s(\w*):\s(\w*)\((.*)\)\s-\sxclim\sversion:\s(\d*\.\d*\.\d*[a-zA-Z-]*)",
+        out.attrs["history"],
+    ).groups()
+
+    date = dt.datetime.fromisoformat(matches[0])
+    assert dt.timedelta(0) < (dt.datetime.now() - date) < dt.timedelta(seconds=3)
+    assert matches[1] == "tas"
+    assert matches[2] == "func"
+    assert matches[3] == "da=tas, arg1=1, arg2=<list>, arg3=None"
+    assert matches[4] == __version__

xclim/testing/tests/test_indicators.py

@@ -145,7 +145,7 @@ def test_attrs(tas_series):
     assert txm.cell_methods == "time: mean within days time: mean within years"
     assert f"{dt.datetime.now():%Y-%m-%d %H}" in txm.attrs["history"]
     assert "TMIN(da=tas, thresh='5 degC', freq='YS')" in txm.attrs["history"]
-    assert f"xclim version: {__version__}." in txm.attrs["history"]
+    assert f"xclim version: {__version__}" in txm.attrs["history"]
     assert txm.name == "tmin5 degC"
     assert uniIndTemp.standard_name == "{freq} mean temperature"
     assert uniIndTemp.cf_attrs[0]["another_attr"] == "With a value."

xclim/testing/tests/test_sdba/test_processing.py

@@ -45,7 +45,7 @@ def test_jitter_under_thresh():
     assert da[0] > 0
     np.testing.assert_allclose(da[1:], out[1:])
     assert (
-        "jitter(<array>, '1 K', <NoneType>, <NoneType>, <NoneType>) - xclim version"
+        "jitter(x=<array>, lower='1 K', upper=None, minimum=None, maximum=None) - xclim version"
         in out.attrs["history"]
     )