kevinalh commented on code in PR #38015:
URL: https://github.com/apache/airflow/pull/38015#discussion_r1518928827


##########
airflow/decorators/base.py:
##########
@@ -208,11 +208,25 @@ def __init__(
         # since values for those will be provided when the task is run. Since
         # we're not actually running the function, None is good enough here.
         signature = inspect.signature(python_callable)
+
+        # Don't allow context argument defaults other than None to avoid 
ambiguities.
+        if any(
+            param.name in KNOWN_CONTEXT_KEYS and param.default not in (None, 
inspect.Parameter.empty)
+            for param in signature.parameters.values()
+        ):
+            raise ValueError("Context key parameters can't have a default 
other than None")
+
         parameters = [
             param.replace(default=None) if param.name in KNOWN_CONTEXT_KEYS 
else param
             for param in signature.parameters.values()
         ]
-        signature = signature.replace(parameters=parameters)
+        try:
+            signature = signature.replace(parameters=parameters)
+        except ValueError:
+            raise ValueError(
+                "The function signature broke while assigning defaults to 
context key parameters. "
+                "You need to change their position in the parameter list"

Review Comment:
   What about this message? Made it print the signatures of both the current 
and replaced signatures so that users can see what's going on, plus the 
Python-generated error message (in case there's another reason this may break 
in the future, although I can't think of one at the moment).
   
   ```
   The function signature broke while assigning defaults to context key 
parameters.
   
   The decorator is replacing the signature
   > run_task(input: 'str', logical_date: 'str | None', logical_date_2: 'str', 
*args, **kwargs)
   
   with
   > run_task(input: 'str', logical_date: 'str | None' = None, logical_date_2: 
'str', *args, **kwargs)
   
   which isn't valid: non-default argument follows default argument
   ```
   
   Hopefully this isn't too verbose!



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to