librespot-org / librespot

Open Source Spotify client library
MIT License
4.84k stars 610 forks source link

[dev] core: spclient: parse lyrics #1084

Closed gdesmott closed 1 year ago

gdesmott commented 1 year ago

Safe users from doing all the parsing themselves.

roderickvd commented 1 year ago

Thanks for this! This has been missing from metadata. Would you be so kind to move the struct and parsing there? The idea is that SpClient is a fairly low-level interface and like the other responses, they are parsed in metadata.

gdesmott commented 1 year ago

Ah ok, I wasn't sure what was the proper way to integrate this.

Sure I can move it to metadata but I'm not sure how to do so. For example, looking at Track, it uses a Message from protocol::metadata but we don't have those for lyrics.

How should we do that?

roderickvd commented 1 year ago

It's been a while since I last worked on metadata -- I can't remember if there are any structs there that do not parse a protobuf. I would not mind if you did not implement the Metadata trait but do a custom implementation of request and/or parse. If you see any way to make it more generic, feel free to do so.

gdesmott commented 1 year ago

Something like this then?

gdesmott commented 1 year ago

@roderickvd I fixed your comments

roderickvd commented 1 year ago

Looks good to me. If you could add a changelog entry, ready to merge. Feel free to fix that new clippy lint along the way to fix the CI.

gdesmott commented 1 year ago

done

gdesmott commented 1 year ago

All clippy warnings are now fixed.

roderickvd commented 1 year ago

Great ❤️