search

tehkillerbee / mopidy-tidal

Tidal Backend plugin for Mopidy
Apache License 2.0
93 stars 28 forks source link

Feat/lazy connection #104

Closed 2e0byo closed 1 year ago

2e0byo commented 1 year ago

This PR finally reimplements lazy connection logic on top of the new tidal api.

I've also added a few more tests to justify the assertions in the readme, and cleaned up the test suite a bit.

The only public member of backend is now on_start. I think _session should become public (session) as it's actually used from outside, but I've not made the change (I'll add it if you agree).

2e0byo commented 1 year ago

CI failure is spurious; could you re-run? Or it will when you merge anyway. It fails with 404 when uploading coverage, but the tests pass fine.

[Edit ignore, I found a typo to correct and the next run fixed it]

2e0byo commented 1 year ago

Would you merge a PR renaming _session to session? I don't want to break anything, but it's only used internally by mopidy tidal. But the backend exports it, so IMHO it should be a normal attribute (it was kindof weird that legitimately private methods had public names).

tehkillerbee commented 1 year ago

@2e0byo I would say that all improvements (and cleanup) of the existing code is welcome.

Most (almost all) of the mopidy-tidal code has been written by other authors, so I cannot say for sure why it was written in the way it was. But there are probably many things that works well enough and therefore they are never changed. At least that was my approach, when I started maintaining the project. After all, it was more important that I could continue using the module with Python3 :)