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

Allow exiting object selection early #3085

Merged
merged 1 commit into from
Dec 2, 2018

Conversation

jackwilsdon
Copy link
Member

@jackwilsdon jackwilsdon commented Nov 28, 2018

Fixes #3083.

I've updated the CLI documentation to mention the new q option, but I'm not exactly a wordsmith 😁 let me know if you can think of some better phrasing!

@jackwilsdon jackwilsdon force-pushed the modify-skip-remaining branch 2 times, most recently from 7e1ed21 to 3463573 Compare November 28, 2018 17:07
@jackwilsdon jackwilsdon force-pushed the modify-skip-remaining branch from 3463573 to bed3abd Compare November 28, 2018 17:17
@sampsyo
Copy link
Member

sampsyo commented Dec 1, 2018

Awesome! This implementation looks good, and I think the documentation text is perfect as well.

There's just one small thing that concerns me: whether the word "quit" alone is enough to suggest the right behavior to the user. I can imagine people seeing that for the first time and imagining that it will cancel the whole operation rather than applying to all the items that have been selected so far.

The thing is, that might be fine—because if someone intends to abort the whole process, they probably will do it on the first prompt, when the behavior is identical to "quit." And given that I don't have a great idea for an alternative to "quit," maybe we should just roll with it! But I thought I'd get your thoughts on the matter before we hit the green button.

Other than that reservation, I think this is good to go!

@jackwilsdon
Copy link
Member Author

Yeah I wasn't sure on "quit" either, but couldn't think of another term to use 😖 The only alternative I can think of right now is cancel, but I'm not sure if that makes it clear to the user that it will stop the selection process as opposed to just skipping the current object.

@sampsyo
Copy link
Member

sampsyo commented Dec 1, 2018

There's always something excessively verbose like "skip remaining," I guess! But you're right; "quit" is probably the best bet. 👍

@jackwilsdon
Copy link
Member Author

jackwilsdon commented Dec 2, 2018

What do you think about changing it to cancel?

$ beet modify title="New Title"
Modifying 1 items.
Artist - Album - Title
  title: Title -> New Title
Really modify, move and write tags? (Yes/no/select) s   

Artist - Album - Title
  title: Title -> New Title
Really modify, move and write tags? (yes/no/cancel) c

Edit: after reading what I just wrote, I realised "cancel" kind of implies that none of the previously selected modifications are applied, which isn't true. I'll let you merge this if you're happy with quit 😄

@sampsyo
Copy link
Member

sampsyo commented Dec 2, 2018

Yes indeed, I think "quit" is a little better than "cancel." Merging! Thank you for your help on this (and for thinking through the wording with me)!! ✨ 🚀

@sampsyo sampsyo merged commit ca359d7 into beetbox:master Dec 2, 2018
@jackwilsdon jackwilsdon deleted the modify-skip-remaining branch December 2, 2018 19:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants