-
Notifications
You must be signed in to change notification settings - Fork 14.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow Param to support a default value of None
#19034
Conversation
@@ -24,6 +24,16 @@ | |||
from airflow.exceptions import AirflowException | |||
|
|||
|
|||
class NoValueSentinel: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was running into differing id
's with a naive object()
sentinel, so I went with this class and used isinstance
for comparison. Side benefit, allows for a better str/repr.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we now have like five different sentinel class implementations in the code base. I have one for DAG(schedule_interval)
. Probably should unify them somewhat into airflow.utils.types
at some point.
Further reading for those interested: PEP 661: Sentinel Values
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On higher level it makes sense but would defer to @msumit
lgtm |
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease. |
b3729eb
to
08db24d
Compare
None
default Param; refactor Param default handlingNone
7239d17
to
64e54ce
Compare
(cherry picked from commit ee4fb07)
(cherry picked from commit ee4fb07)
Allow Param to support a default value of
None
.This also stores the
default
value invalue
instead ofdefault
, since we later will update the resolved value.closes: #18909