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

Replace askyesno() with click.confirm() #296

Merged
merged 1 commit into from
Dec 2, 2020
Merged

Conversation

jwodder
Copy link
Member

@jwodder jwodder commented Dec 2, 2020

It turns out that askyesno() is already provided by a library we're already using. The only advantage of the former is that it supports repeated prompting when no default is given and the user enters a empty line, but we weren't using that feature anyway.

@jwodder jwodder added the patch Increment the patch version when merged label Dec 2, 2020
@codecov
Copy link

codecov bot commented Dec 2, 2020

Codecov Report

Merging #296 (07d8885) into master (c7aac72) will decrease coverage by 0.17%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #296      +/-   ##
==========================================
- Coverage   82.13%   81.95%   -0.18%     
==========================================
  Files          55       53       -2     
  Lines        4640     4594      -46     
==========================================
- Hits         3811     3765      -46     
  Misses        829      829              
Flag Coverage Δ
unittests 81.95% <100.00%> (-0.18%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
dandi/girder.py 87.88% <100.00%> (ø)
dandi/tests/test_girder.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c7aac72...07d8885. Read the comment docs.

@yarikoptic
Copy link
Member

and there would be no side-effect of some kind if we use dandi as python package, and thus not going through click's cmdline parsing/handling? just want to make sure

@jwodder
Copy link
Member Author

jwodder commented Dec 2, 2020

@yarikoptic As far as I'm aware, the only "tie-in" the function has to the rest of Click is that uses a Click-specific exception if the user hits Cntrl-C.

@yarikoptic
Copy link
Member

Hm, not entirely 100% optimal (outside code, e.g. soon to be introduced https://github.com/datalad/datalad/blob/master/datalad/support/parallel.py) might need to catch that exception too then in some very rare cases when underlying dialog happens to be embedded. But I think it is tolerable and we could later shim it if need arises. Let's proceed

@yarikoptic yarikoptic merged commit 511143d into master Dec 2, 2020
@yarikoptic yarikoptic deleted the askyesno-confirm branch December 2, 2020 18:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch Increment the patch version when merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants