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

Update __init__.py #3220

Merged
merged 6 commits into from Apr 22, 2019

Conversation

Projects
None yet
2 participants
@rain0r
Copy link
Contributor

commented Apr 17, 2019

Also fetch genres for single tracks via query.

Update __init__.py
Also fetch genres for single tracks via query.

@rain0r rain0r referenced this pull request Apr 17, 2019

Closed

LastGenre for Singletons? #3219

@sampsyo

This comment has been minimized.

Copy link
Member

commented Apr 17, 2019

Hi! This is a nice start, but doing the same actions to both albums and items matching the same query is likely to do a bunch of redundant work in most cases. For example, the query beatles will match both some albums and the tracks on those albums, so the command would fetch the genres for those tracks twice.

I think the right thing to do is to put this behind a command-line option: one that switches between album and item mode. As you'll see in most other plugin-provided commands, there's an -a flag that switches to album mode. We could do the same thing, but make this the default—and require something like -A to switch back to item mode.

@rain0r

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2019

Added the parser options, what do you think?

@sampsyo

This comment has been minimized.

Copy link
Member

commented Apr 18, 2019

Cool! This arrangement, however, does make it possible for albums and tracks to be enabled simultaneously. It would be nice if this were a proper switch: i.e., track and album mode negated one another, and only one could be active at a time.

Also, can you please call the track mode "item" mode, for uniformity with other commands?

Finally, could you please look into the Travis failures, add documentation, and write a quick changelog entry?

rain0r added some commits Apr 19, 2019

@rain0r

This comment has been minimized.

Copy link
Contributor Author

commented Apr 20, 2019

Cool! This arrangement, however, does make it possible for albums and tracks to be enabled simultaneously. It would be nice if this were a proper switch: i.e., track and album mode negated one another, and only one could be active at a time.

Also, can you please call the track mode "item" mode, for uniformity with other commands?

Finally, could you please look into the Travis failures, add documentation, and write a quick changelog entry?

Done

@sampsyo

This comment has been minimized.

Copy link
Member

commented Apr 20, 2019

Looking good! If you can bear with me, I have a few more suggestions:

  • A clean way to do the "mutual excursion" thing would be to have the CLI flags control a single option variable. (This is the case in most other plugins and commands, if you take a look at that code.) To both option constructors, you can add dest='album'. Then, the -A option can use action='store_false' and the -a one can use action='store_true'. Then, the code for the command can just use if opts.album: and else: for the two cases.
  • In the documentation, can you please explain how to use the new switch? That is, say that the command matches albums by default. If you want to look up genres for individual tracks (and singletons) instead, use -A.
  • Please see the way that other plugin features are described in the changeling. We use ReST syntax like this: :doc:`/plugins/lastgenre`: Some change here..
- Improved doc and changelog
- Cleaner implementation of mutual excursion of the command line
arguments.
@rain0r

This comment has been minimized.

Copy link
Contributor Author

commented Apr 21, 2019

No problem, I'm happy if someone has an eye on code quality ;-)

Now it works like this:

$ beet lastgenre -a
# albums
$ beet lastgenre -A
# items 
$ beet lastgenre 
# albums

Also updated the doc.

@rain0r

This comment has been minimized.

Copy link
Contributor Author

commented Apr 21, 2019

If this goes through, #3219 can also be closed.

@sampsyo

This comment has been minimized.

Copy link
Member

commented Apr 22, 2019

Awesome! Thank you for your persistent work on this. 🐟 Merged!

@sampsyo sampsyo merged commit b7d3ef6 into beetbox:master Apr 22, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

sampsyo added a commit that referenced this pull request Apr 22, 2019

sampsyo added a commit that referenced this pull request Apr 22, 2019

sampsyo added a commit that referenced this pull request Apr 22, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.