search

FUMR / music-service-async-interface

GNU Lesser General Public License v2.1
1 stars 0 forks source link

Consider adding audio_quality property to Track #7

Closed github-actions[bot] closed 3 years ago

github-actions[bot] commented 4 years ago

_This issue has been automatically created by todo-actions based on a TODO comment found in music_service_async_interface/init.py:192. It will automatically be closed when the TODO comment is removed from the default branch (master)._

wvffle commented 4 years ago

Instead of creating enum, we can make it a simple uint. The higher the value - the lower the quality. That way we can ensure that track of quality of 0 is the best no matter the name in implementation.

JuniorJPDJ commented 4 years ago

I used enum so I can parse JSON strings as enum values, most of streaming services use string as quality indicator, not int. If streaming service will use int or anything else it will still be possible to attach it as enum value. I wanted to connect this indicator with from-app names, so It's clear for user what quality it corresponds to. Example is here: https://github.com/FUMR/tidal-async/blob/master/tidal_async/api.py#L20

Enum names are names of quality in Tidal's UI and enum values is real data sent to app via API.

If we would just return int it would be hard to guess what it really means without reading the docs.

JuniorJPDJ commented 4 years ago

Also what if streaming service would add new quality, better than before? We would need to change every int and users could miss this and use the same int as other value. It already happened in the past.

wvffle commented 4 years ago

Yeah but the problem appears when some service would have more quality options than we predicted.

Creating a PR to the generic repo when implementing such service isn't a problem but storing this as an uint as I proposed would make things easier on the implementation side, right?

Also what if streaming service would add new quality, better than before?

Then we can start with some bigger integer as 100?

JuniorJPDJ commented 4 years ago

If service had more quality options than we predicted your solution still wouldn't work, as we would need to convert this JSON string values to ints. IMO it's not making anything easier and It actually can make much things harder to understand. Still - I haven't heard of service that don't advertise their quality selection in UI, there's always some sort of selector of quality.

JuniorJPDJ commented 4 years ago

BTW. This discussion should happen in #6 I already did pull request to implement this and I'm curious of your opinion.

JuniorJPDJ commented 3 years ago

I think it's not the best idea for interface, as not always it will be easily fetchable information before you try to download track - Not every music service will provide us that information.