beets icon indicating copy to clipboard operation
beets copied to clipboard

add lots of type hints

Open amogus07 opened this issue 2 months ago • 4 comments

Description

I added type hints ~and refactored the modules to fix import cycles~

To Do

  • [ ] Documentation
  • [ ] Changelog
  • [ ] Tests

amogus07 avatar Oct 24 '25 00:10 amogus07

Thank you for the PR! The changelog has not been updated, so here is a friendly reminder to check if you need to add an entry.

github-actions[bot] avatar Oct 24 '25 00:10 github-actions[bot]

Hey, thanks a lot for the contribution, really appreciate the effort you’ve put into this!

This PR is quite large, which makes it a bit difficult to review thoroughly. Would you mind breaking it down into smaller, more focused commits or separate PRs? For instance, you could move file relocations or refactors into their own commits to make the changes easier to follow. You can take a look at the PR linked below for an example of how to structure these kinds of refactors.

It also looks like this PR touches on multiple areas of the codebase. To keep things maintainable and easier to review, it’s best if each PR focuses on a single concern or goal, rather than combining several changes at once.

As a side note, have you had a chance to look at #6119? It seems to address some of the same issues you’re working on here, it might be worth aligning with that effort to avoid overlap.

Thanks again for your work on this! We really appreciate your recent efforts!

semohr avatar Oct 24 '25 09:10 semohr

Apologies for my language - I realise I sound dismissive and terse there - I am tired and about to go to sleep, so this is my fault, not your changes <3

I take that most of these type adjustments stem from you using pyright to type check this project. In an ideal world, pyright and mypy would show the same issues, but they don't in reality. This project uses mypy as our type checker, so changes should address mypy's requirements rather than pyright's. Some of these type hints may be fixing issues that pyright sees but mypy doesn't, because it manages to resolve the types.

Would you mind running mypy on your changes to see which type issues actually need addressing for our setup? That would help focus the PR on changes that are truly necessary. Please use resolve_type to see whether a type is missing for a variable.

Thanks again for your effort on this - I really do appreciate it!

snejus avatar Oct 29 '25 05:10 snejus

@snejus Thank you for taking the time to review this! Yeah, I probably should've left it as a draft for now, I pretty much expected some changes would have to be reverted. I did realize that this project uses mypy, but either it doesn't play well with my editor (Zed) or just it's LSP isn't as good as Basedpyright, which is what I'm using. As for the "redundant" type annotations, that's just my personal preference. Sure, you can live without them, but using I just don't like the concept of inferred types at all. They can change without me noticing if I accidentally reuse a variable for an unintended purpose. My goal is to get beets closer to a state where it complies with mypy's strict mode. Although I might suggest migrating to a different type checker like basedpyright at some point. On the other hand, we could wait until pyrefly becomes stable, which is significantly faster then both mypy and basedpyright. I will go over your feedback in detail when I have time and use it as guidance for future contributions.

amogus07 avatar Oct 29 '25 08:10 amogus07