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

play: add --random option #2350

Closed
wants to merge 3 commits into from
Closed

Conversation

diomekes
Copy link

This adds feature request #795 and part of #2305.

It's the same as #2304, but I closed that one due to updates to the random plugin (and because I didn't branch my master fork to make changes, you live you learn).

As discussed in #2304, #795 and #1948, the best option would be to add a "random" query prefix, but that's too advanced for me (at least for right now) to develop, so I did this instead.

@nathdwek
Copy link
Member

nathdwek commented Dec 30, 2016

First and foremost, thank you for your work and commitment to this feature, and welcome to beets!

I'd like to see this feature implemented very much, but to be honest I don't like it being done this way. I think we should bite the bullet and implement randomness at the query level.

This would be useful for many other operations, right out of the box (the latest example being #2352 )! Furthermore, as I mentioned in #2304, I don't find the use of with nostdout() very elegant here, along with the three imports.

That being said, I perfectly understand that you would not take on such a project, and I am perfectly open to diverging opinions on this.

@sampsyo
Copy link
Member

sampsyo commented Dec 30, 2016

Agreed—thank you for following up with this after #2322!

Let's explore what it would take to make this part of the query syntax. The tricky thing here is that a query like beatles rand:5 or something would currently be parsed as an AndQuery of an ordinary SubstringQuery query and a new RandomQuery or somesuch. The "and" part doesn't make much sense for random sampling.

Instead, what we want is more like our current sort operators. They modify the whole query that they appear in, so beatles year+ does not parse to an AndQuery. Maybe we need a generic mechanism for "query modifiers" like sorting?

@diomekes
Copy link
Author

diomekes commented Dec 30, 2016

I agree with the nostdout() being ugly, so I "fixed" it (I didn't see your comment on #2304).

If we are able to make it into a query (I'm willing to help, I just don't have any experience), would we still have access to the --equal-chance and --time options provided by random?

I'm guessing it would involve multiple queries under the proposed RandomQuery, or a separate totaltime query that could be used alongside a random query.

@sampsyo
Copy link
Member

sampsyo commented Dec 30, 2016

Yeah. We'd have to come up with good syntax for it. Here are some (probably bad) ideas:

  • rand.t:5 for 5 minutes
  • rand.e:5 for 5 objects with equal chance
  • randtime:5
  • or if you like ASCII, 5~ for 5 random objects and 5t~ or something for the time version

@diomekes
Copy link
Author

I think we do need a mechanism for modifiers like sorting (are there other types of modifiers already?). Sorting requires a field and + or -. Shuffling a query, instead of sorting, could require a ?. For example:

ls beatles ? would return a random beatles item (if 1 is the default, or something before ? could be a requirement).
ls beatles 10? would return ten.
ls beatles 0:30? would return 30 minutes.

I have no idea how to implement this, I was just looking at query.py and queryparse.py, so I don't know if this is something a plugin can do or if it has to be in the core.

@sampsyo
Copy link
Member

sampsyo commented Dec 31, 2016

Right! No, sorting is the only thing currently like this, so the query handling is currently hard-coded to only support sorting. We'd need to refactor things so that sorting, instead of being a special snowflake, is just a specific instance of a more general category.

The ? syntax is definitely suggestive! And it's probably safe for most shells too—at least if they're configured to leave wildcards intact when they don't match any files.

@diomekes
Copy link
Author

diomekes commented Jan 6, 2017

After further thought, it seems like we need to do two things:

  1. A query modifier that randomizes the query, maybe ? as I suggested earlier.
  2. Give beets the ability to output either a certain number of results or a list that stays within a time limit.

The first one seems like it could be a part of beets current sorting with + or -, excluding the field requirement.

The second one, as far as I can tell, isn't a part of beets yet, although it can be done with random. I don't think a plugin should do it though, for the same reasons we're trying to de-plugin random's functionality. Unless, of course, it's a plugin that modifies the query syntax with a prefix, something like beet ls 5:beatles.

Has there been any discussion, as far as you know @sampsyo, of limiting beets' output?

@sampsyo
Copy link
Member

sampsyo commented Jan 7, 2017

Good point! Randomizing and truncating could indeed be decoupled.

The closest thing here is #750, which we come up with in the context of the web interface. But the basic functionality is the same: limiting the number of results returned from a query.

@jtpavlock
Copy link
Contributor

@diomekes any interest in continuing this?

@diomekes
Copy link
Author

@jtpavlock Yes, but it's beyond my abilities. I did try to figure it out way back when, but just couldn't find out how to do it the right way (making randomness a query instead of just baked into the "play" plugin).

@jtpavlock
Copy link
Contributor

I completely understand, it's not exactly what you signed up for when you opened this PR 😄

I also appreciate that you've generated a worthwhile discussion to the future implementation of this feature.

I'm going to close this since it seems we'd prefer the implementation to be a much bigger and different idea than what you originally intended/were able to work on. If you'd like to continue in the discussion or have any further ideas, I suggest moving to #795. And if you ever want to come back and attempt to tackle this, feel free to open another pull request!

@jtpavlock jtpavlock closed this Jul 10, 2020
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.

4 participants