Importer typehints and small importer refactor #5611
Conversation
Makes this more readable in my opinion, we also now have typehints for the import state.
we have slightly different behavior.
in util. Also run poe format
|
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. |
|
There is one failing tests that has, as far as I can see, nothing to do with any of the changes in the PR. Some help would be highly appreciated here. |
|
I'm now looking at the test as that's something that I merged earlier, apologies! |
The return type of the stage decorator should in theory be `T|None` but the return of task types is not consistent in its usage. Would need some bigger changes for which I'm not ready at the moment.
semohr
force-pushed
the
importer-typehints-and-refactor
branch
from
9bbb22d to
34101c2
Compare
|
I just merged the master into this branch, but the test still seems to be broken. Tried to fix it but tbh I have no idea what the issue is here, as I don't have much insights into how the lyrics plugin works. It is also difficult to debug as the issue does not occur locally for me. From my side feel free to merge this! I might make another PR in the near future for some changes related to the pipeline, especially modernizing it a bit. |
|
Apologies, I haven't yet addressed the lyrics issue... 🤦🏼 |
|
The fix is up #5618 |
|
Don't stress it! Was just checking it again and wasn't able to fix it myself. |
|
It's been merged - feel free to rebase / merge now! Will have a look at the types you added now, coincidentally I've been doing the same in my fork, so I'm intrigued! 😉 |
|
Awesome! If you want feel free to check my async-import branch too. I added some more type hints there too, will maybe make a PR once this is a bit more polished and it is fully backwards compatible. |
|
See the last two bits in unresolved conversations - otherwise we'll be good to go! |
Description
Hello y'all.
One of the dev from the beets-flask app here. We are extending the
ImportSessionin our application to get the interactive imports working in our application. Overall that was not a too pleasant experience as a ton of typehints are missing in that area of the beets code. This PR wants to fix this ;)Changes
Moved importer state logic into a dataclass. Is clearer and more isolated now in my opinion, we are not using it but I found it quite confusing when looking at it.
Added a ton of typehints to the
importer.pyfile. Predominantly theImportSession,ImportTask(and derivatives) and pipeline stage decorators are now (hopefully) completely typed. Additionally I fixed some typhint issues in theutil.__init__.pyfile, mainly related to thePathLiketype.If you have some strong opinions about any of the changes feel free to revert or edit the PR.
Best,
Sebastian
P.S: I noticed that beets does not use Abstract Base Classes is there any reason for that or is it just of historic nature? I think some Base Classes could be adapted to use an ABC metaclass quite easily.
To Do
docs/changelog.rstto the bottom of one of the lists near the top of the document.)