search

beetbox / beets

music library manager and MusicBrainz tagger
http://beets.io/
MIT License
12.57k stars 1.8k forks source link

Update annotations from Python 3.8 support #5292

Open Serene-Arc opened 3 weeks ago

Serene-Arc commented 3 weeks ago

As stated in #5283, there are some annotation updates now that Python 3.8 is the minimum supported version. These should be done for all modules.

bal-e commented 2 weeks ago

I'd like to help with this. I see that the main idea is to use list / dict / tuple wherever possible instead of the typing.* aliases -- are there any more annotations I should look into? Furthermore, I know that there's ongoing work (#5223) for improving type annotations in beets.util -- should I avoid it (or any other files) for now?

Serene-Arc commented 2 weeks ago

Updating the annotations should bed a separate PR but the goal is to add typing to the entire project eventually!

I do need to double check that these typings are supported now. I think it's 3.8 but not totally sure. You're welcome to open a pr and check though!

bal-e commented 2 weeks ago

Updating the annotations should be a separate PR

I understand, but I figured fixing up those files right now would cause merge conflict hell for the existing PRs. I'm happy to fix them up anyways, just wanted to get some advice on it.

Alright, I'll have a shot at this and see what happens. If it's still too early, we can revive the PR once Python 3.9 becomes the minimum.

Serene-Arc commented 2 weeks ago

Yes, updating the functions to be annotated is difficult and requires a bunch of changes that makes a lot of merge conflicts. That's why it's being done piecemeal, a file at a time.

bal-e commented 2 weeks ago

According to the documentation of typing.List, replacing List[T] with list[T] is introduced in PEP 585 with Python 3.9. However, that PEP also mentions that these nicer type annotations are available from Python 3.7 onward using from __future__ import annotations, which is already pretty common in the beets codebase. So I'm going to continue making the PR, relying on __future__.annotations everywhere. We can try dropping it once our minimum version bumps to 3.9.

Serene-Arc commented 2 weeks ago

Cool, it'll have to wait for that. I wonder what did change because we've been using that future import and nothing has broken?

Python 3.8 is ending in like three months, then we'll go to 3.9.

Serene-Arc commented 2 weeks ago

@bal-e maybe hold off, I'm working on a pathlib integration that will change a lot of signatures.

bal-e commented 2 weeks ago

Oh cool! Integrating pathlib was the original reason I started contributing here, but it's a really big change and I thought I'd work up to it first. You're a lot more familiar with the codebase than me, so I would prefer seeing your PR over trying to make my own for it.

If you've already got a decent amount of work done for it, I can stick to making changes completely unrelated to paths (e.g. I'm currently looking at beets/autotag/mb.py, where I couldn't see much path-related stuff at all). On the other hand, if you're just starting out, then the fixes I'd make as part of this PR should also give you slightly cleaner mypy runs (I'm adding some more annotations as I go), which can be very helpful. Let me know what you'd prefer.

Serene-Arc commented 2 weeks ago

@snejus and I have been discussing it through the day, so we're getting to it. Just trying to merge #5227 before I submit a draft of the pathlib integration PR. It'll be a lot of changes so there'll definitely be time for your input if you'd like to take a look at it! I'll try and remember to ping you when I make the draft.