Skip to content
This repository has been archived by the owner on Sep 17, 2023. It is now read-only.

Refactor Hit struct to be an enum? #10

Open
vrobweis opened this issue Sep 21, 2021 · 4 comments
Open

Refactor Hit struct to be an enum? #10

vrobweis opened this issue Sep 21, 2021 · 4 comments
Labels
question Further information is requested

Comments

@vrobweis
Copy link
Contributor

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.

genius-rs/src/search.rs

Lines 5 to 11 in 283c082

#[derive(Deserialize, Debug)]
pub struct Hit {
pub index: String,
#[serde(rename = "type")]
pub hit_type: String,
pub result: Song,
}

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
Copy link
Owner

alt-art commented Sep 21, 2021

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
Copy link
Contributor Author

vrobweis commented Sep 21, 2021

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
Copy link
Owner

alt-art commented Sep 21, 2021

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
Copy link
Owner

alt-art commented Sep 21, 2021

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.

@alt-art alt-art added the question Further information is requested label May 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants