Skip to content
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

fix confusion between prompt and don't prompt in the plotnft CLI #17951

Merged
merged 2 commits into from
May 4, 2024

Conversation

arvidn
Copy link
Contributor

@arvidn arvidn commented May 2, 2024

Purpose:

Fix bugs where dont_prompt turned into prompt, inverting the meaning.

Enforcing named parameters did not help avoid this mistake. It probably makes sense to unify all functions to just use the dont_prompt form of this bool, and not the inverse.

While I was at it, I also harmonized the confirmation function in plotnft to use the cli_confirm() utility function. This was a separate commit and affects indentation, please review commits independently and ignore whitespace.

Current Behavior:

chia wallet plotnft create -y prompts for confirmation.

New Behavior:

chia wallet plotnft create does not prompt for confirmation.

@arvidn arvidn added CLI Fixed Required label for PR that categorizes merge commit message as "Fixed" for changelog labels May 2, 2024
@arvidn arvidn requested a review from altendky May 2, 2024 11:06
@arvidn arvidn marked this pull request as ready for review May 2, 2024 12:07
@arvidn arvidn requested a review from a team as a code owner May 2, 2024 12:07
@arvidn arvidn closed this May 2, 2024
@arvidn arvidn reopened this May 2, 2024
@arvidn arvidn closed this May 3, 2024
@arvidn arvidn reopened this May 3, 2024
Copy link
Contributor

github-actions bot commented May 3, 2024

File Coverage Missing Lines
chia/cmds/plotnft.py 0.0% lines 97
chia/cmds/plotnft_funcs.py 0.0% lines 92, 94-95, 104-113, 258-271
Total Missing Coverage
28 lines 28 lines 0%

Copy link

Pull Request Test Coverage Report for Build 8935001258

Details

  • 0 of 28 (0.0%) changed or added relevant lines in 2 files are covered.
  • 50 unchanged lines in 14 files lost coverage.
  • Overall coverage decreased (-0.01%) to 90.673%

Changes Missing Coverage Covered Lines Changed/Added Lines %
chia/cmds/plotnft.py 0 1 0.0%
chia/cmds/plotnft_funcs.py 0 27 0.0%
Files with Coverage Reduction New Missed Lines %
chia/_tests/simulation/test_simulation.py 1 96.5%
chia/daemon/keychain_proxy.py 1 64.98%
chia/harvester/harvester_api.py 1 73.02%
chia/rpc/rpc_server.py 1 87.67%
chia/farmer/farmer.py 1 72.56%
chia/wallet/util/wallet_sync_utils.py 1 86.54%
chia/daemon/client.py 1 73.33%
chia/full_node/full_node_api.py 2 80.84%
chia/data_layer/data_layer.py 2 84.77%
chia/data_layer/data_layer_wallet.py 2 91.26%
Totals Coverage Status
Change from base Build 8931619722: -0.01%
Covered Lines: 97984
Relevant Lines: 108007

💛 - Coveralls

@arvidn arvidn requested a review from emlowe May 3, 2024 13:45
@arvidn arvidn added ready_to_merge Submitter and reviewers think this is ready and removed coverage-diff labels May 3, 2024
@pmaslana pmaslana merged commit 1661ce2 into main May 4, 2024
1039 of 1046 checks passed
@pmaslana pmaslana deleted the plotnft-cli-prompt branch May 4, 2024 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLI Fixed Required label for PR that categorizes merge commit message as "Fixed" for changelog ready_to_merge Submitter and reviewers think this is ready
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants