search

InputUsername / listenbrainz-rs

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

Implement GET /1/playlist/(playlist_mbid) #19

Closed Kernald closed 6 months ago

Kernald commented 6 months ago

This builds upon the feature/new-endpoints branch, adding the endpoint to actually fetch playlists (as other existing endpoints actually don't return tracks, as far as I'm aware). I also annotated some jspf fields that aren't always provided by ListenBrainz.

This is my first actual Rust bit of code, so there might be issues with it - although I tried to mimic what's around it so it should most likely be fine. I just realised when opening this PR that my editor formatted a bit too much - happy to revert those changes if there's any actual interest into getting this merged, but I'm not sure what the status of this upstream branch actually is, as it hasn't been updated in quite while?

shymega commented 6 months ago

I would rather not merge any formatting changes into the branch, given the formatter has affected other functions.

I'm just wondering how we can avoid merging the aforementioned changes, though...

Kernald commented 6 months ago

Formatting changes removed - how do you feel about me opening a PR just to apply repo-wide formatting changes to avoid this kind of friction in the future? As far as I understand, cargo fmt is the most common way of formatting Rust code, so most people's editors will follow that?

shymega commented 6 months ago

Formatting changes removed - how do you feel about me opening a PR just to apply repo-wide formatting changes to avoid this kind of friction in the future? As far as I understand, cargo fmt is the most common way of formatting Rust code, so most people's editors will follow that?

I spotted one more fmt change.

I will leave that decision up to @InputUsername. The formatting is generally done by rustfmt, yes - we should also look into EditorConfig.

shymega commented 6 months ago

Other than formatting, I'm happy with the PR. Will approve when the final review is resolved.

InputUsername commented 6 months ago

I'm not sure what the status of this upstream branch actually is, as it hasn't been updated in quite while?

I honestly kind of forgot about the branch as I haven't needed any of the additional endpoints myself, and judging by the issues others have not either. Looking at it now, this branch actually seems to be based on version 0.4.1 which is over 3 years old :sweat_smile:

Still happy to try to at least get a subset merged into main though. @shymega I guess we can just try to merge whatever is currently implemented into main and keep the issue open?

how do you feel about me opening a PR just to apply repo-wide formatting changes to avoid this kind of friction in the future? As far as I understand, cargo fmt is the most common way of formatting Rust code, so most people's editors will follow that?

I have a sneaking suspicion main is already formatted correctly, it's just this branch that is really old, but feel free to create a PR if we've missed anything!

Kernald commented 6 months ago

Nits addressed, thanks!

Thanks for the update on the status of this branch too. As you probably have guessed, I have an interest in some of those playlist-related endpoints, so would definitely be keen on seeing this merged and eventually released. Happy to help in any way I can!

Regarding formatting, it seems you're right and main is properly formatted, I'll disregard that then :-)

InputUsername commented 6 months ago

LGTM!

@shymega I think the way forward is merging this PR, then rebasing+merging feature/new-endpoints into main but keeping the branch? Then we can release whatever is there now and implement the other endpoints at some other time.

shymega commented 6 months ago

Hey @InputUsername sorry, was busy. I've reviewed the remaining PRs, but I think rebasing and merging is a good step forward, as long as we don't rebase on main. We can always cherry-pick if necessary.

InputUsername commented 6 months ago

Hey @InputUsername sorry, was busy. I've reviewed the remaining PRs, but I think rebasing and merging is a good step forward, as long as we don't rebase on main. We can always cherry-pick if necessary.

@shymega No worries! I've already taken the liberty of merging main into feature/new-endpoints so after the other MRs we should be ready for the next release (0.8.0?) with some of the new endpoints, how does that sound to you?