-
Notifications
You must be signed in to change notification settings - Fork 260
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
GH-5768: Better pyflyte boolean parsing #2764
GH-5768: Better pyflyte boolean parsing #2764
Conversation
9658cbe
to
a0fe865
Compare
Signed-off-by: Thomas Newton <thomas.w.newton@gmail.com>
Signed-off-by: Thomas Newton <thomas.w.newton@gmail.com>
a0fe865
to
65807ff
Compare
Signed-off-by: Thomas Newton <thomas.w.newton@gmail.com>
f4b89c0
to
928bee3
Compare
Signed-off-by: Thomas Newton <thomas.w.newton@gmail.com>
b844fa3
to
604f07c
Compare
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.
Thank you. This makes sense. I have only one small comment.
Signed-off-by: Thomas Newton <thomas.w.newton@gmail.com>
0cad3a5
to
34466a3
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2764 +/- ##
===========================================
+ Coverage 78.98% 92.13% +13.14%
===========================================
Files 194 19 -175
Lines 19807 1309 -18498
Branches 4130 0 -4130
===========================================
- Hits 15645 1206 -14439
+ Misses 3443 103 -3340
+ Partials 719 0 -719
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
There is a lint error. otherwise LGTM
Signed-off-by: Eduardo Apolinario <eapolinario@users.noreply.github.com>
Tracking issue
Closes flyteorg/flyte#999
Why are the changes needed?
The current boolean parsing behaviour is confusing for default True arguments.
What changes were proposed in this pull request?
Add
--no_...
and--no-...
variants for boolean flags as per the recommendation at https://click.palletsprojects.com/en/8.1.x/options/#boolean-flagsHow was this patch tested?
Wrote new unit tests to cover it.
Setup process
Screenshots
Check all the applicable boxes
--help
should be automatically taken care of by theclick
library.Related PRs
Docs link