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

Selective apply of functions to items #1843

Merged
merged 12 commits into from Feb 4, 2016
Merged

Conversation

pkess
Copy link
Collaborator

@pkess pkess commented Jan 28, 2016

as suggested in #1838 i implemented a more generic way to let the user select a list of items to apply an action for

@sampsyo implementation for modify and move command will follow if this approach meets your requirements

@pkess
Copy link
Collaborator Author

pkess commented Jan 28, 2016

I added the interactive mode for both modify and move command now and added a changelog entry. Please review
TODO:

  • Add test case for modify and timid mode
  • Maybe add a configuration option to put move in timid mode always

out_items = []
choice = input_options(
('y', 'n', 's'), False,
'%s (Yes/No/Selective)?' % prompt)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A tiny thing, but I might find the verb "Select" more intuitive than the noun "Selective".

@sampsyo
Copy link
Member

sampsyo commented Jan 29, 2016

Cool! This is feeling like the right direction. It might make sense to try to unify all the different places where we do confirmation (if there are any others—remove, perhaps?) and funnel them through a similar codepath.

@pkess
Copy link
Collaborator Author

pkess commented Jan 29, 2016

I changed "Selective" to "Select items" now, as it more intuitive.
I also changed the docstring. Ok now?

@sampsyo
Copy link
Member

sampsyo commented Jan 29, 2016

Cool, that helps! Thanks!

If we're going to have a noun in the prompt, though, it should be "Select {}s".format('album' if album else 'item'), though, since this function could be called on both Album and Item objects. In fact, maybe the function name should avoid using item also.

Thanks for being with all the terminology bike-shedding! We're definitely getting closer.

out_items = []
choice = input_options(
('y', 'n', 's'), False,
'%s (Yes/No/Select items)?' % prompt)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again on the detail-obsession topic: most of the other confirmation prompts in beets seem to have the form "Question? (A/b/c)" rather than "Question (A/b/c)?", so we should probably try for consistency with the placement of the question mark.

Also, we usually format this kind of prompt using just the initial letters, one of which is capitalized to indicate that it's the default. Of course, that isn't as easy to read when the options aren't just Y and N, which are self-evident. It's possible that we need some creativity here—how can we make it clear that there's a default, that you can just type one letter, and yet also that there's a third option?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's right. I would like a way which will let you print a help like suggested above

@sampsyo
Copy link
Member

sampsyo commented Jan 31, 2016

On the broader question of what the prompt should look like: let's stick with something simple for now. The plain (colorized) output from input_options will do just fine:

>>> beets.ui.input_options(('yes', 'no', 'select'))
[Y]es, No, Select?

The capitals, brackets, and colors help suggest that Y and N are options, that Y is the default, and that you can also choose to "select" without being too verbose.


Short, off-topic rant: Overall, I'm worried about adding too much complexity to what should be a very simple prompt for most uses. The majority of people will just want to say "yes" or "no" to a listing, so giving a long list of options to read seems like a shame. Git is certainly a counterexample, but IMO option overload is one of the reasons git is so unusable.

moved question mark and removed word "item"
@pkess
Copy link
Collaborator Author

pkess commented Feb 3, 2016

Hi,

i changed the prompt to a more common one.
If i understand you, you don't want to extend the interface to support those additional options mentioned above. So i would not implement this at the moment.

I also added some code to catch a KeyboardInterrupt in pkess/catch_keyboardinterrupt. Are there any contradictions to implement it like this? It would suppress the Backtrace on output and exit in a clean way. I could also add a line to tell the user s.th. like "Abort due to CTRL-C"

@pkess
Copy link
Collaborator Author

pkess commented Feb 3, 2016

If you are ok with this implementation, i would like to merge it to master.

@sampsyo
Copy link
Member

sampsyo commented Feb 4, 2016

Great!

Regarding KeyboardInterrupt, I believe we already handle this at the top level (in main), so I don't think we need another handler here.

@sampsyo
Copy link
Member

sampsyo commented Feb 4, 2016

Looks good to me after a review, so please do feel free to merge.

If you can, at some point, please add a brief note to the docs for modify describing the feature. It will be good to have at least one description written down of the feature's intent and exactly how to use it.

@pkess pkess merged commit 89d38ca into beetbox:master Feb 4, 2016
pkess added a commit that referenced this pull request Feb 4, 2016
Conflicts:
	docs/changelog.rst

closes #1843
@pkess pkess deleted the selective_modify2 branch February 4, 2016 16:58
pkess added a commit to pkess/beets that referenced this pull request Feb 4, 2016
sampsyo added a commit that referenced this pull request Feb 4, 2016
sampsyo added a commit that referenced this pull request Feb 7, 2016
Most urgently, this function doesn't just work on *items*; it works on
arbitrary objects (and, in particular, albums).
sampsyo added a commit that referenced this pull request Feb 7, 2016
To make the docstirng fit in with the standard style.
@Vrihub
Copy link
Contributor

Vrihub commented Nov 24, 2016

If you can, at some point, please add a brief note to the docs for modify describing the feature. It will be good to have at least one description written down of the feature's intent and exactly how to use it.

Yes, please document modify select feature :)

@pkess
Copy link
Collaborator Author

pkess commented Nov 24, 2016

Hi, i just opend PR #2280 to meet these requirements

sampsyo added a commit that referenced this pull request Nov 25, 2016
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.

None yet

3 participants