Skip to content

Add and fix some typings#85

Open
Serene-Arc wants to merge 9 commits intoNeurrone:mainfrom
Serene-Arc:typings_fix
Open

Add and fix some typings#85
Serene-Arc wants to merge 9 commits intoNeurrone:mainfrom
Serene-Arc:typings_fix

Conversation

@Serene-Arc
Copy link
Copy Markdown

A few small fixes to the typings for some functions, mainly adding some but also fixing a couple incorrect typings.

@Neurrone
Copy link
Copy Markdown
Owner

Thanks for the PR. Could you resolve the merge conflicts?

@Serene-Arc
Copy link
Copy Markdown
Author

Done!

If you're amenable, I would also like to start some work on tests and then some file sorting improvements. I don't know if you remember, but I started similar work a couple years ago. The codebase changed enough that I closed that PR request but I think it would be a big improvement for the import process.

Comment thread beetsplug/api.py


def make_request(url: str) -> bytes:
def make_request(url: str) -> AnyStr | None:
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

The usage of AnyStr looks problematic - it doesn't do anything if it is only used once in the function definition, and is also not recommended for use. See https://docs.python.org/3/library/typing.html#typing.AnyStr

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

It's there because that's the return for the generic of that function. In practice though, it's probably fine to pin it to bytes instead since it shouldn't return a string, or else I can make it the replacement for AnyStr recommended in the docs.

What would you prefer?

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Lets go with bytes for now? We could always adjust it later, but bytes won't be deprecated in the upcoming Python releases.

Comment thread beetsplug/audible.py Outdated
@Neurrone
Copy link
Copy Markdown
Owner

Having more tests would be really useful. I tried adding tests but the current plugin isn't really set up well for that.

Ideally, it should be possible to have lists of results from Audible, metadata from a book to be imported and we can check that the plugin assigns the correct Audible result.

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