search

InputUsername / listenbrainz-rs

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

Added playlist_create body structs #32

Closed RustyNova016 closed 3 months ago

RustyNova016 commented 4 months ago

The current implementation require a full jspf struct to insert a playlist, while most of that information get discarded.

So I made a series of specialized structs for the creation specifically

RustyNova016 commented 4 months ago

BTW, the documentation errors aren't mine, and probably better into a separate PR

shymega commented 3 months ago

Once #29 is merged, the target branch for this PR should be changed to main, and rebased against main.

@RustyNova016 I don't want to conflate matters, so would it be OK if I leave the rebasing to you?

Then we can look at merging.

InputUsername commented 3 months ago

@RustyNova016 Could you rebase this PR so that it only includes commit https://github.com/InputUsername/listenbrainz-rs/pull/32/commits/da9d776e5e8d0a21fdf8591d050038828b65f9b3? The other commits have landed in main now.

shymega commented 3 months ago

@InputUsername To save time, I've rebased locally and pushed to my fork - this look good to you?

InputUsername commented 3 months ago

@InputUsername To save time, I've rebased locally and pushed to my fork - this look good to you?

Looks fine to me!

shymega commented 3 months ago

Looks like CI failed. Looking into it.

shymega commented 3 months ago

Fixed.

shymega commented 3 months ago

The doctests failed due to the changes in this PR, so I feel it's OK to keep the fix here. I will squash the last two commits together, though. Rebasing now.

shymega commented 3 months ago

This might still need actioning: https://github.com/RustyNova016/listenbrainz-rs/issues/19#issue-2291466134

RustyNova016 commented 3 months ago

Should be good to go! I was busy today, and mostly forgot about doing the annotation fix.

I'm sure there might have some obscure options left missing, but the documentation not usage made me discover them.

shymega commented 3 months ago

Should be good to go! I was busy today, and mostly forgot about doing the annotation fix.

I'm sure there might have some obscure options left missing, but the documentation not usage made me discover them.

Once we resolve the review above, I'll squash the last commit, and we can merge.

RustyNova016 commented 3 months ago

Oh I thought I only put a comment, not a review. Sorry for that

shymega commented 3 months ago

I've squashed the commits in this PR into one and signed off the commit. I'll re-request a review from @InputUsername and myself, then we can merge!

shymega commented 3 months ago

Builds and tests pass on my end. Awaiting CI.

RustyNova016 commented 3 months ago

Nice! Is there's still stuff needed before do 8.0? Just wondering.

34 will be delayed as I'm busy for the next few weeks, so if y'all rather release what we have already, it's fine

InputUsername commented 3 months ago

Nice! Is there's still stuff needed before do 8.0? Just wondering.

34 will be delayed as I'm busy for the next few weeks, so if y'all rather release what we have already, it's fine

I think we're good to go for the 0.8.0 release. And no worries, that was the original plan anyway I think :smile:

@shymega could you give everything a sanity check? If everything is OK I'll roll out the release.

shymega commented 3 months ago

@InputUsername I'll try and have a look, but got an interview with an embedded company this week, so need to prep everything for that. At least, I think it's an interview. Given tests pass, the main thing I can think of is breaking-changes checks. So we can announce that to downstream users.

InputUsername commented 3 months ago

@InputUsername I'll try and have a look, but got an interview with an embedded company this week, so need to prep everything for that. At least, I think it's an interview. Given tests pass, the main thing I can think of is breaking-changes checks. So we can announce that to downstream users.

No worries, good luck with the interview, obviously focus on that!

The version bump should already imply breaking changes - Rust "breaks" semver by treating 0.x releases as major versions - but good call, I'll check to make sure breaking changes are mentioned in the changelog. I'll also check if the added methods actually work haha.

RustyNova016 commented 3 months ago

There's the I at least know that user_listens got its api changed to remove the useless time range part