InputUsername / listenbrainz-rs

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

Update to the UserListensResponse models to add MBID mapping and documentation #22

Open RustyNova016 opened 6 months ago

RustyNova016 commented 6 months ago

The current response models lack any MBID mapping information that would appear on the request, making it impossible to determine if the listen is mapped or not.

I've added the missing fields, and added documentation comments for all the other fields of the response.

shymega commented 6 months ago

Would rather not merge this, the history isn't linear. Is it intended for #24 to supersede this PR?

InputUsername commented 6 months ago

I agree, seems like a duplicate of #24 but including the added docs?

RustyNova016 commented 6 months ago

I agree, seems like a duplicate of #24 but including the added docs?

You asked on #20 to separate the docs, so I separated them. #24 can be merged now, but #22 is meant to be for the documentation update.

Should I had waited for #24 to go through?

InputUsername commented 6 months ago

I agree, seems like a duplicate of #24 but including the added docs?

You asked on #20 to separate the docs, so I separated them. #24 can be merged now, but #22 is meant to be for the documentation update.

Should I had waited for #24 to go through?

No that's fine, we can merge this one later 😄

shymega commented 6 months ago

We can merge this once the other PR goes through, but the duplicate commit history should be double-checked before merging.

RustyNova016 commented 6 months ago

This one not only need to wait #24 but also work on the whole documentation update like discussed in #20. Unless another branch get created for the documentation, but that would either mean going before or after #25, or else we would have to move the new documentation to the new files.

I feel like this PR has become more of a reminder of that there is work to put somewhere at one point than actually mergable.

shymega commented 6 months ago

You can remove the unused commits in a rebase, and once #24 is merged, rebase main onto this branch, and push. I can see how the commits ended up this way here, but it does - and can - mess up history.

shymega commented 5 months ago

After approval from @InputUsername (ref: #27), I've rebased this branch against main, squashed the commits, and fixed my nitpick. This is just so we can get this great PR into main.

shymega commented 5 months ago

@InputUsername We're all good to merge now.

cc: @RustyNova016 - you'll need to do git pull --rebase.

InputUsername commented 5 months ago

Looks good.

@shymega should we wait until after 0.8.0 is released so we can add docs to everything in one go (e.g. for 0.8.1)?

shymega commented 1 month ago

Wait for v0.8.1. Sorry, didn't see your ping.