-
Notifications
You must be signed in to change notification settings - Fork 333
support passing optional date through CLI #3273
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
base: master
Are you sure you want to change the base?
support passing optional date through CLI #3273
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3273 +/- ##
==========================================
+ Coverage 83.35% 92.85% +9.50%
==========================================
Files 347 18 -329
Lines 28791 812 -27979
Branches 2960 0 -2960
==========================================
- Hits 23999 754 -23245
+ Misses 3956 58 -3898
+ Partials 836 0 -836 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
I think it should be here - flytekit/flytekit/interaction/click_types.py Line 517 in b0efdba
|
@kumare3 Alright, what do you think about this approach? |
I'm hoping we can move forward with this |
flytekit/interaction/click_types.py
Outdated
if self._python_type is datetime.date and isinstance(value, datetime.datetime): | ||
# Click produces datetime, so converting to date to avoid type mismatch error | ||
value = value.date() |
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 can just modify here and do not need to add a new DateType
. While it already deal with the conversion for datetime.date
type, what we need to do is to also trigger this conversion for Optional[datetime.date]
and datetime.date | None
type.
The following should work:
is_union = typing.get_origin(self._python_type) in (typing.Union, types.UnionType)
args = typing.get_args(self._python_type)
is_date_type = (
(is_union and datetime.date in args)
or self._python_type is datetime.date
)
if is_date_type and isinstance(value, datetime.datetime):
value = value.date()
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.
Ok, that's fair. I've made the change, so hopefully we can get this merged in soon.
Signed-off-by: Blake <[email protected]>
Signed-off-by: Blake <[email protected]>
1757a6e
to
79939f4
Compare
Signed-off-by: Blake <[email protected]>
79939f4
to
07981f1
Compare
@blaketastic2 Could you fix CI failure? Thanks! |
Why are the changes needed?
Currently, if we have a
workflow
with an optional date parameter, we cannot call this from the CLI.Workflow definition:
Executing the workflow:
Produces this error:
What changes were proposed in this pull request?
We are now making sure to use the non-None variant when trying to do the conversion.
Summary by Bito
This pull request enhances the CLI's handling of optional date parameters by improving type conversion, specifically addressing datetime to optional date type conversions to prevent type mismatch errors. These enhancements aim to improve user experience in workflows requiring date parameters.