Skip to content

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