-
-
Notifications
You must be signed in to change notification settings - Fork 16
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
base: master
Are you sure you want to change the base?
Conversation
…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
This looks pretty good to me, but I would love it if @amCap1712 could also have a quick look. |
I'm a bit confused by this change. This changes the format used for submission. Why are only 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. |
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 |
I see. Let's wait for feedback from @mayhem and @amCap1712 . My concerns are mainly two:
I see 2. coming up as a question for every pull request opened on external players :) |
Hi all! Sorry for the delay. About the listen submission format, the |
I see, so 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? |
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. |
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.