InputUsername / listenbrainz-rs

ListenBrainz API bindings for Rust
https://crates.io/crates/listenbrainz
MIT License
11 stars 6 forks source link

response_type! macro doesn't allow for top level Array responses #33

Open RustyNova016 opened 4 months ago

RustyNova016 commented 4 months ago

The current response_type! macro is built in such a way that it only JSON that has an object at its top level can be parsed. However, new endpoints were made that return a top level Array instead: https://listenbrainz.readthedocs.io/en/latest/users/api/popularity.html

While I'm able to implement the endpoints, I have no experience in macros. Would be nice to have it fixed so I can implement them.

InputUsername commented 4 months ago

Thanks for raising this!

Not sure if it'd be of any help, but there is the following documentation on writing macros: https://veykril.github.io/tlborm/

RustyNova016 commented 4 months ago

Temporary fixed it with a generic struct. Which may(?) be better than a macro, as the responses aren't edited at all from the LB documentaion

But yeah... Learning macro is something that needs a lot of time still

/// A generic response type for listenbrainz responses
#[derive(Deserialize)]
pub struct ListenbrainzResponse<T: Deserialize> {
    pub rate_limit: Option<RateLimit>,
    pub data: T
}

impl<T: Deserialize> ResponseType for ListenbrainzResponse<T> {
fn from_response(response: Response) -> Result<Self, Error> {
    let response = Error::try_from_error_response(response)?;

    let mut inner_data: T = response.json()?;
    let rate_limit = RateLimit::from_headers(&response);

    Ok(Self {
        rate_limit,
        data: inner_data,
    })
}
}
InputUsername commented 4 months ago

Temporary fixed it with a generic struct. Which may(?) be better than a macro, as the responses aren't edited at all from the LB documentaion

But yeah... Learning macro is something that needs a lot of time still

/// A generic response type for listenbrainz responses
#[derive(Deserialize)]
pub struct ListenbrainzResponse<T: Deserialize> {
    pub rate_limit: Option<RateLimit>,
    pub data: T
}

impl<T: Deserialize> ResponseType for ListenbrainzResponse<T> {
fn from_response(response: Response) -> Result<Self, Error> {
    let response = Error::try_from_error_response(response)?;

    let mut inner_data: T = response.json()?;
    let rate_limit = RateLimit::from_headers(&response);

    Ok(Self {
        rate_limit,
        data: inner_data,
    })
}
}

Ooh I like that approach, does this work for every response? Because the less macro use the better, honestly :smile:

RustyNova016 commented 4 months ago

It clippy doesn't complain... So It's fine. Then you just need to put your Vec into a type:

// ---------- popularity/recording

pub type PopularityRecordingResponse = Vec<PopularityTopRecordingsForArtistResponseItem>;

#[derive(Debug, Deserialize, Serialize, Clone, PartialEq, Eq)]
pub struct PopularityRecordingResponseItem {
    pub recording_mbid: String,
    pub total_listen_count: u64,
    pub total_user_count: u64
}

But well... A PR is better than words, so I'm going to make a draft PR to expose the changes

InputUsername commented 4 months ago

Perfect!

shymega commented 4 months ago

I really like this approach. I detest macros, generics FTW!

Really pleased with the code sample above @RustyNova016 - great job. Let's get the remaining PRs in, then work out a strategy on refactoring to use generics. We should open a tracking issue, and link each PR in with that.

RustyNova016 commented 3 months ago

Macros are great, but not developer friendly. I got an issue in the musicbrainz crate and can't fix it because of a complicated macro system...

Eitherway, I need to do the generic way for #34, so that's one less to do.