search

piotrmurach / tty-prompt

A beautiful and powerful interactive command line prompt
https://ttytoolkit.org
MIT License
1.47k stars 136 forks source link

Preserve initial order of selected choices despite the order they were selected in #141

Closed DanielVartanov closed 4 years ago

DanielVartanov commented 4 years ago

Describe the change

Preserve initial order of selected choices despite the order they were selected in

Why are we doing this?

Examplary motivation: I'm writing a deployment script which at one point asks the user which commits to cherry-pick for further deployment. The order of the commits is paramount.

Benefits

multi_select will be a solution to cases where order of chosen items is crucial

Drawbacks

Will have to maintain the option in future

Requirements

Put an X between brackets on each line if you have done the item: [X] Tests written & passing locally? [rubocup reports 2000+ offenses, but they don't seem to be related to this PR] Code style checked? [X] Rebased with master branch? [X] Documentation updated?

coveralls commented 4 years ago

Coverage Status

Coverage decreased (-0.2%) to 97.162% when pulling 8e3b2b298a67f6477f4536d3dba60f7b7f792a5b on DanielVartanov:multi-select-preserve-order into 26877a57deeca824f3d1375f152780e3345ba929 on piotrmurach:master.

coveralls commented 4 years ago

Coverage Status

Coverage increased (+0.002%) to 97.343% when pulling 58c2197f0d4d2eff2819914f08fe818a58c46f14 on DanielVartanov:multi-select-preserve-order into b330a32eb0d509b9d515d59819b9745e0cea0b42 on piotrmurach:master.

DanielVartanov commented 4 years ago

Noted the coverage thing, will add a test for preserve_order: false too

DanielVartanov commented 4 years ago

@piotrmurach not sure why did it report coverage twice with opposite results, but just in case I've added a test for the case when preserve_order: false (see https://github.com/piotrmurach/tty-prompt/pull/141/commits/62cde120311334d9cfe23c63d8f8fe8589844834), please let me know whether I should keep it in the PR. Thanks in advance.

DanielVartanov commented 4 years ago

Ah, great, that's even better then! I added it as an option only because I wasn't too sure. Awesome, I will update the PR then :+1:

piotrmurach commented 4 years ago

Thanks for bringing this issue up. We need to explore all avenues before settling on a solution 😄

DanielVartanov commented 4 years ago

@piotrmurach thanks Piotr, I only ask not to postpone it too much as my deployment automation awaits for it :-)

DanielVartanov commented 4 years ago

@piotrmurach I've explored both options: adding index to Choice in order to make Choices sortable and adding selected to Choice, I'll create PR's for both and this one will be updated to make the fixed behaviour default.

DanielVartanov commented 4 years ago

This pull request is now updated.

DanielVartanov commented 4 years ago

PRs https://github.com/piotrmurach/tty-prompt/pull/142 and https://github.com/piotrmurach/tty-prompt/pull/143 represent two paths @piotrmurach has suggested.

But this PR can be merged right away to solve the initial problem.