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

Download by series, download by author #184

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

taxilian
Copy link

Implements #183

My python is more than a bit rusty; I tried to follow existing patterns, but let me know if there is anything you need me to fix.

@mkb79
Copy link
Owner

mkb79 commented Feb 13, 2024

It looks quite good so far. One point should be considered again. What if someone is looking for a title and author. Then only audio books should be displayed where the title and author match. Currently, he would search by title or author and add the results!

@mkb79 mkb79 self-requested a review February 13, 2024 15:58
@taxilian
Copy link
Author

That is true; I assumed that was the way you wanted to do it since that's how it's set up with ASNs and titles, but if you want I can probably figure out how to do an intersection instead

@mkb79
Copy link
Owner

mkb79 commented Feb 14, 2024

Maybe the if match condition for authors, title and series should be merged. So first add only asins where authors, title and series match and then make the questionary?!

help="title of the audiobook (partial search)"
)
@click.option(
"--author", "-a",
Copy link
Owner

Choose a reason for hiding this comment

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

The -a option is already used by --asin.

Copy link
Author

Choose a reason for hiding this comment

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

Oops =] I thought I'd looked for that.

All things considered, I think the more natural assumption would be to have the different lists merged before checking -- I just haven't had time to come back to this since then and I'm not sure how soon I can

Copy link
Owner

@mkb79 mkb79 Feb 22, 2024

Choose a reason for hiding this comment

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

I think I'll have time this weekend to rework some things and then merge your request.

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