Closed InputUsername closed 3 months ago
@shymega in case you missed it, could you take a look at this?
The JSPF format doesn't correspond to the official. And according to the playlist creation endpoint it seems a lot of fields are optional when in the code they aren't.
It seems to be fine for playlist fetching tho.
Hey, sorry - you're right, I did miss this. The only thing I'd say is that it's a fairly large PR, and I wonder if we can split it into 'groups' based on endpoints. For example, one PR for '/user/*' endpoints, and so on. It's quite a lot to review without making mistakes.
We also need to rebase against main
. There's a merge conflict on my end...
Hey, sorry - you're right, I did miss this. The only thing I'd say is that it's a fairly large PR, and I wonder if we can split it into 'groups' based on endpoints. For example, one PR for '/user/*' endpoints, and so on. It's quite a lot to review without making mistakes.
At this point that would be better to just ditch the branch and go directly into the #25 refactor.
BTW, I didn't encounter any merge conflict while making my own 8.0 branch. Odd
Hey, sorry - you're right, I did miss this. The only thing I'd say is that it's a fairly large PR, and I wonder if we can split it into 'groups' based on endpoints. For example, one PR for '/user/*' endpoints, and so on. It's quite a lot to review without making mistakes.
At this point that would be better to just ditch the branch and go directly into the #25 refactor.
BTW, I didn't encounter any merge conflict while making my own 8.0 branch. Odd
It would be better to split PRs. Easier to track. Having so many changes in one PR, will just make life difficult.
I had a merge conflict when rebasing against main
.
Hi, sorry for the slow response, prioritizing my master's thesis right now :smile:
I agree that splitting up is probably a good idea.
At this point that would be better to just ditch the branch and go directly into the https://github.com/InputUsername/listenbrainz-rs/issues/25 refactor.
Not sure, this branch contains a bunch of additional endpoints that we definitely want to keep. But doing the refactor first and then porting over the new endpoints in parts sounds like a good idea.
@shymega I currently don't have a lot of time for this, could you take charge?
Hi, sorry for the slow response, prioritizing my master's thesis right now 😄
I agree that splitting up is probably a good idea.
At this point that would be better to just ditch the branch and go directly into the #25 refactor.
Not sure, this branch contains a bunch of additional endpoints that we definitely want to keep. But doing the refactor first and then porting over the new endpoints in parts sounds like a good idea.
@shymega I currently don't have a lot of time for this, could you take charge?
I can take a look on weekends. I may be signing a contract for Mon-Wed next week, and then have another part-time thing for the rest of the week.
I need to revert the merge commits used in this PR, and edit the history. Ideally PRs shouldn't have merge commits, and just use the rebase method against main. I will notify the PR here once that's done, and a git pull --rebase
is necessary on local checkouts.
This rebase won't be easy.
@InputUsername Can you provide me with a list of SHAs you want in this PR, and I'll then cherry-pick them, rebase the branches you want in this PR, and rebase against main too.
It's needed because I need to remove the merge commits, but I don't want to miss any of your work. Feel free to email me or state here.
Cheers.
@shymega
In the end, reverting two merge commits would have been messy. As main
has merge commits anyway, I just rebased this PR against main. I've pushed it to my fork.
I had to decide which were the new changes, and old changes. Any queries, feel free to let me know what I've missed.
Since #32 is based on this branch and merge in it, feel free to tell me if there's any change to do on it
So far, you don't need to do anything. I'm waiting for @InputUsername to assess my branch, and check we can force-push to this PR.
I don't want to break anything, but when we do overwrite this PR with my branch, you may need to rebase #32.
@shymega I had a look (sorry for the delay!) and it seems like your branch is good apart from the fact that the Endpoint::StatsReleaseGroupListeners
variant got removed somehow? Anyway, I think we can go ahead and overwrite this PR, CI should catch any major issues.
Next time I'll make sure not to keep a branch hanging around for over 2 years :sweat_smile:
@shymega I had a look (sorry for the delay!) and it seems like your branch is good apart from the fact that the
Endpoint::StatsReleaseGroupListeners
variant got removed somehow? Anyway, I think we can go ahead and overwrite this PR, CI should catch any major issues.Next time I'll make sure not to keep a branch hanging around for over 2 years 😅
How does it look now? It should now be ready to force-push.
I also found a compile error which I've fixed - missing brace.
@shymega I had a look (sorry for the delay!) and it seems like your branch is good apart from the fact that the
Endpoint::StatsReleaseGroupListeners
variant got removed somehow? Anyway, I think we can go ahead and overwrite this PR, CI should catch any major issues. Next time I'll make sure not to keep a branch hanging around for over 2 years 😅How does it look now? It should now be ready to force-push.
I also found a compile error which I've fixed - missing brace.
Good catch - let's go for it!
Just done it. Let's see how CI goes now.
This PR implements some of the API endpoints from #4:
/1/user/(user_name)/similar-users
-Client::user_similar_users
;/1/user/(user_name)/similar-to/(other_user_name)
-Client::user_similar_to
./1/user/(playlist_user_name)/playlists
-Client::user_playlists
;/1/user/(playlist_user_name)/playlists/createdfor
-Client::user_playlists_created_for
;/1/user/(playlist_user_name)/playlists/collaborator
-Client::user_playlists_collaborator
;/1/playlist/(playlist_mbid)
-Client::get_playlist
./1/playlist/create
-Client::playlist_create
;/1/playlist/(playlist_mbid)/delete
-Client::playlist_delete
;/1/playlist/(playlist_mbid)/copy
-Client::playlist_copy
./1/stats/release-group/(release_group_mbid)/listeners
-Client::release_group_listeners
./1/user/(user_name)/followers
-Client::user_followers
;/1/user/(user_name)/following
-Client::user_following
;/1/user/(user_name)/follow
-Client::user_follow
;/1/user/(user_name)/unfollow
-Client::user_unfollow
.