fixed false positive warning: "UNUSED KEYWORD: flag", for functions that dont...

fixed false positive warning: "UNUSED KEYWORD: flag", for functions that dont use/need a flag (eg. SaQC.tools.copy)

saqc/core/core.py

@@ -220,11 +220,12 @@ class SaQC(FuncModules):
         return data.to_df(), self._translator.backward(flags)
 
     def _wrap(self, func: Callable):
-        """ Enrich a function by special saqc-functionality.
+        """Enrich a function by special saqc-functionality.
 
         For each saqc function this realize
             - the source-target workflow,
             - regex's in field,
+            - use default of translator for ``to_mask`` if not specified by user,
             - translation of ``flag`` and
             - working inplace.
         Therefore it adds the following keywords to each saqc function:
@@ -232,12 +233,13 @@ class SaQC(FuncModules):
 
         The returned function returns a Saqc object.
         """
+
         def inner(
             field: str,
             *args,
             target: str = None,
             regex: bool = False,
-            flag: ExternalFlag = BAD,
+            flag: ExternalFlag = None,
             inplace: bool = False,
             **kwargs,
         ) -> SaQC:
@@ -245,13 +247,11 @@ class SaQC(FuncModules):
             if regex and target is not None:
                 raise ValueError("explicit `target` not supported with `regex=True`")
 
-            # kwargs['flag'] = self._translator(flag)
-            # kwargs.setdefault('to_mask', self._translator.TO_MASK)
-            kwargs = {
-                "flag": self._translator(flag),
-                "to_mask": self._translator.TO_MASK,
-                **kwargs,
-            }
+            kwargs.setdefault("to_mask", self._translator.TO_MASK)
+
+            # translation
+            if flag is not None:
+                kwargs["flag"] = self._translator(flag)
 
             # expand regular expressions
             if regex:

saqc/funcs/tools.py

@@ -310,8 +310,8 @@ def plot(
 
     fig_kwargs : dict, default None
         Keyword arguments controlling figure generation. In interactive mode,
-        ``None`` defaults to ``{"figsize": (16, 9)}`` to ensure a proper figure size,
-        in store-mode ``None`` defaults to a empty dictionary.
+        ``None`` defaults to ``{"figsize": (16, 9)}`` to ensure a proper figure size
+        in store-mode.
 
     save_kwargs : dict, default {}
         Keywords to be passed on to the ``matplotlib.pyplot.savefig`` method, handling
@@ -367,7 +367,7 @@ def plot(
         data=data,
         field=field,
         flags=flags,
-        level=kwargs["flag"],
+        level=kwargs.get('flag', BAD),
         max_gap=max_gap,
         stats=stats,
         plot_kwargs=plot_kwargs,

[David Schäfer]

I don't like the changes to inner introduced here. Could you please explain, why they are necessary @plamb.

My points are:

• Changing the function signature to flag=None makes sense as it is type hinted to ExternalFlag and well, BAD is not an external flag. But in the end we need we need 'real' flags (i.e. not None) at some point. And I think here is the perfect place to ensure this. I don't think we should delegate this responsibility to downstream functions (see the change to saqc/funcs/tools.py).
• I don't like the .setdefault pattern as it mutates the input arguments to the function, making it impure for no apparent reason. I always try to follow the idea, that function should not change the outside world (i.e. arguments and global variables), if not absolutely necessary. And here such changes are not necessary.

IDK, the commit message suggests, that this commit silences the unused keyword warnings. I agree, that this should be fixed, but instead of complicating the main machinery here, we should rather complicate the checks.

[Bert Palm]

ok, i get your point. The main issue is that we shouldnt inject flag if the function does not have it in its signature. so we can change it to:

def inner(
	field,
	*args,
	target,
	regex,
	inplace,
	**kwargs
    ):

	kwargs = {**kwargs}  # copy 
	
	if 'flag' in kwargs:
	    kwargs["flag"] = self._translator(kwargs['flag'])
	    
	kwargs.setdefault("to_mask", self._translator.TO_MASK)

	...

[Bert Palm]

i prefer kwargs.setdefault("to_mask", self._translator.TO_MASK) over kwargs = {"to_mask": self._translator.TO_MASK, **kwargs}, because both are the same, namely setting a default. but the latter do it implicitly. It is especially dangerous if we change the order to kwargs = {**kwargs, "to_mask": self._translator.TO_MASK}, because now we always get the default !

[David Schäfer]

The main issue is that we shouldnt inject flag if the function does not have it in its signature

Why not? It should end up in the funcions kwargs, which should be alright, I think? And what do we get from flags[field] = None?

[David Schäfer]

It is especially dangerous if we change

Hmm, IDK... changing the order of things is usually dangerous