alt-art / genius-rs

Rust library that allows interact with Genius API
https://crates.io/crates/genius-rs
MIT License
7 stars 3 forks source link

Refactor Hit struct to be an enum? #10

Open vrobweis opened 2 years ago

vrobweis commented 2 years ago

Love this crate. API wrappers are the unsung heroes of any application ecosystem, and I'm glad to see any improvements to Rust's application-level situation. Having said that, I'm curious if this library can map to some common Rust idioms.

In this case, as of version 0.4.0, this library still uses a struct to represent a Hit, or search result from the Genius API. https://github.com/alt-art/genius-rs/blob/283c0825e88eeaef6cdf79ea9afdf801600f249a/src/search.rs#L5-L11

Can this be changed? serde allows the deserialization of enums, and if there are even types other than "song" that would appear in the type field, an enum could be used to represent them. It would certainly be much easier to use than having to read all the public fields of each type.

I couldn't find a complete list of "type"s that Genius search hits can have, but since they seem to only currently support a song type, it is likely they may simply have future proofed, intending to update their database later on to distinguish between music videos and songs, which they currently do not do. For the time being, a single-variant, #[non-exhaustive] enum could represent this perfectly.

alt-art commented 2 years ago

I don't use enums because I have no experience with them, the examples of the enum representation of serde seems disheartening, if you have an example of this I will check.

Genius API supports many types of searches, but they only offer developers a small music search, in a no longer future I will support these types.

vrobweis commented 2 years ago

Without confirming (I am away from a computer at present), a possible representation could look like:

#[derive(Deserialize, Debug)] 
#[serde(tag = "type", content = "result")]
#[non_exhaustive]
pub enum Hit {
    #[serde(alias = "song")]
    Song(Song)
}

This uses serde's support for adjacent tagging to handle possible outcomes. This sample code may not be ideal, as it does remove support for the index value, but by my recollection, that information isn't really something consumers of the crate would use, since they would presumably get whatever results they need in the Vec of Hits. If the index is used for pagination, that's a crate implementation detail.

alt-art commented 2 years ago

Hmm, the way you did it would be possible:

#[derive(Deserialize, Debug)] 
#[serde(tag = "type", content = "result")]
#[serde(tag = "type", content = "album")]
#[non_exhaustive]
pub enum Hit {
    #[serde(alias = "song")]
    Song(Song)
    #[serde(alias = "album")]
    Album(Album)
}

?

alt-art commented 2 years ago

I reviewed the API responses for certain types of search and I think that even, so I will have to write another structure to treat some cases. See multiple types search, articles search and tiny song search. The tiny music search will be separated from the others responses.