Skip to content
This repository has been archived by the owner on Sep 13, 2023. It is now read-only.

546 boolean options flags #600

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

ykasimov
Copy link
Contributor

Fixes #546

@ykasimov ykasimov requested a review from a team as a code owner January 27, 2023 19:17
@ykasimov ykasimov temporarily deployed to external January 27, 2023 19:17 — with GitHub Actions Inactive
@aguschin
Copy link
Contributor

Hi @ykasimov! Approved PR run, and added you once again to the reviewers. You'll get notifications for PRs in this repo, but you are able now to approve yours :)

@ykasimov ykasimov temporarily deployed to external February 3, 2023 11:53 — with GitHub Actions Inactive
@codecov
Copy link

codecov bot commented Feb 3, 2023

Codecov Report

Base: 86.24% // Head: 86.24% // No change to project coverage 👍

Coverage data is based on head (355fa90) compared to base (f1c4d3b).
Patch has no changes to coverable lines.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #600   +/-   ##
=======================================
  Coverage   86.24%   86.24%           
=======================================
  Files         107      107           
  Lines        9641     9641           
=======================================
  Hits         8315     8315           
  Misses       1326     1326           
Impacted Files Coverage Δ
mlem/cli/utils.py 87.33% <ø> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@ykasimov ykasimov temporarily deployed to external February 3, 2023 14:26 — with GitHub Actions Inactive
@aguschin
Copy link
Contributor

aguschin commented Feb 7, 2023

@mike0sv is this ready to be merged?

@mike0sv
Copy link
Contributor

mike0sv commented Feb 9, 2023

@ykasimov you still need to address the second part of my comment in the original issue

Need to be careful though because we also might need to alter option name. Eg if is_on has default value False, it makes sense that --is_on will make it True. But if is_on default value is True, we should have something like --not_is_on as an option name

Otherwise you either will not be able to change default value via CLI or it will be name for a flag. Also need some tests for this

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for boolean options as flags (instead of string True/False values)
3 participants