Skip to content

Conversation

@ashb
Copy link
Member

@ashb ashb commented Apr 16, 2020

In #8219 we noticed that we couldn't set a default=0 because of the
and v check. The "add_argument" function in python avoid this by using
**kwargs, but we want type checking so can't directly use the same
there.

This uses the same pattern that configparser does to allow falsey (0,
False) and even None as valid values, distinct from not being passed.


Make sure to mark the boxes below before creating PR: [x]


In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.
Read the Pull Request Guidelines for more information.

@ashb ashb requested review from dimberman and kaxil April 16, 2020 10:37
@kaxil
Copy link
Member

kaxil commented Apr 16, 2020

One of the tests failed but might be just a flaky test:
``|
FAILED tests/jobs/test_local_task_job.py::TestLocalTaskJob::test_mark_failure_on_failure_callback
= 1 failed, 2594 passed, 46 skipped, 4 xpassed, 6 warnings in 1267.93s (0:21:07) =

@ashb
Copy link
Member Author

ashb commented Apr 16, 2020

Flakey test addressed (hopefully) in #8405

In apache#8219 we noticed that we couldn't set a `default=0` because of the
`and v` check. The "add_argument" function in python avoid this by using
**kwargs, but we want type checking so can't directly use the same
there.

This uses the same pattern that configparser does to allow falsey (0,
False) and even `None` as valid values, distinct from not being passed.
@ashb ashb force-pushed the allow-falsey-default-args branch from d067acc to d9a007c Compare April 16, 2020 15:14
@ashb
Copy link
Member Author

ashb commented Apr 16, 2020

Bored waiting for travis to run the K8s tests for a cli change.

@ashb ashb merged commit 37473d2 into apache:master Apr 16, 2020
@ashb ashb deleted the allow-falsey-default-args branch April 16, 2020 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants