Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
RustyNova016 opened this issue May 8, 2024 · 7 comments · May be fixed by #34
Open

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

RustyNova016 opened this issue May 8, 2024 · 7 comments · May be fixed by #34
Labels
bug Something isn't working

Comments

@RustyNova016
Copy link
Contributor

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

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/

@InputUsername InputUsername added the bug Something isn't working label May 8, 2024
@RustyNova016
Copy link
Contributor Author

RustyNova016 commented May 8, 2024

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

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 😄

@RustyNova016
Copy link
Contributor Author

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

@RustyNova016 RustyNova016 linked a pull request May 8, 2024 that will close this issue
@InputUsername
Copy link
Owner

Perfect!

@shymega
Copy link
Collaborator

shymega commented May 9, 2024

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

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.

@shymega shymega added this to the v8.0 milestone Jul 22, 2024
@InputUsername InputUsername removed this from the v0.8.0 milestone Jul 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants