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

Handle Current JSON Payloads and Update README #18

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

3ricG
Copy link

@3ricG 3ricG commented Oct 1, 2023

Not sure if/when listenbrainz API docs will be updated, but this doesn't conform with the JSON they describe. This change does work with the JSON that's being returned from the API.

Docs for reference: https://listenbrainz.readthedocs.io/en/latest/users/json.html

Also update IRC network in support section of the README.

Note: Apologies for the two other PRs, I mistakenly opened them both off of master.

Eric Gross added 2 commits September 27, 2023 23:54
…be updated, but this doesn't conform with the JSON they describe; however, it does work with the JSON I'm getting back from the API. Docs for reference: https://listenbrainz.readthedocs.io/en/latest/users/json.html
@mayhem
Copy link
Member

mayhem commented Oct 13, 2023

This looks pretty good to me, but I would love it if @amCap1712 could also have a quick look.

@phw
Copy link
Member

phw commented Oct 13, 2023

I'm a bit confused by this change. This changes the format used for submission. Why are only release_mbid and artist_mbids moved from additional_info to an entirely new object? And does this mean all the LB clients already submitting data as documented should be updated now?

IMHO this separate object just increases complexity in client implementation and you will end up with some clients submitting the MBIDs in additional_info and some in mbid_mapping. So if you want to make optimal use of these IDs on the server side you actually would need to read both locations anyway.

Instead of updating the pylistenbrainz client it seems to make more sense to me to update the server accordingly to read what is actually documented.

@3ricG
Copy link
Author

3ricG commented Oct 14, 2023

I see what you're saying, and probably didn't need to modify the format used for submission. The main issue I was trying to solve was that I couldn't get release_mbid and artist_mbids from the API with pylistenbrainz, but that doesn't require a change to the format for submission.

I could be looking at this incorrectly, but I think mbid_mapping should still be included in the listen object that is returned when querying data from the API. It seems to be the only part of the JSON that includes the release_mbid and artist_mbids from what I can see.

@phw
Copy link
Member

phw commented Oct 14, 2023

I see. Let's wait for feedback from @mayhem and @amCap1712 .

My concerns are mainly two:

  1. There are dozens of existing clients that would need to be updated
  2. If it is said that LB wants to change the submission format and go through this hazzle once now, then the current format is still inconsistent as the recording ID is left in additional_info.

I see 2. coming up as a question for every pull request opened on external players :)

@amCap1712
Copy link
Member

Hi all!

Sorry for the delay. About the listen submission format, the mbid_mapping part is not meant to be submitted by the users. It is generated by ListenBrainz server. When you are receiving the data from ListenBrainz server, if the server was able to make a match the response will include both the original submitted additional_info and server generated mbid_mapping part. There are some keys which are common in both parts like artist_mbids, release_mbids, recording_mbids etc. On the website, we usually prefer the data in additional_info over mbid_mapping if present. I hope that clears things up.

@3ricG
Copy link
Author

3ricG commented Nov 1, 2023

I see, so additional_info will only include the various mbid values if the original listen submission also included those values? So in the data I'm looking at, I'm just not seeing that data because the my listens are being submitted without those properties in the additional_info part.

I did change my original commit to not modify how listens would be submitted, 979170f.

Does it still make sense to make the changes to listen.py and utils.py to add mbid_mapping, at least for the response from the server?

@amCap1712
Copy link
Member

Right, if you don't see some values in the additional_info response then those weren't submitted to ListenBrainz in the first place. To make mbid_mapping accessible in the listen object makes sense.

I don't have write access to the repo yet, I'll review and merge once I do.

@georgkrause georgkrause requested a review from amCap1712 January 29, 2024 08:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants