-
Notifications
You must be signed in to change notification settings - Fork 6
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
Update to the UserListensResponse models to add MBID mapping and documentation #22
base: main
Are you sure you want to change the base?
Conversation
Would rather not merge this, the history isn't linear. Is it intended for #24 to supersede this PR? |
I agree, seems like a duplicate of #24 but including the added docs? |
We can merge this once the other PR goes through, but the duplicate commit history should be double-checked before merging. |
This one not only need to wait #24 but also work on the whole documentation update like discussed in #20. Unless another branch get created for the documentation, but that would either mean going before or after #25, or else we would have to move the new documentation to the new files. I feel like this PR has become more of a reminder of that there is work to put somewhere at one point than actually mergable. |
You can remove the unused commits in a rebase, and once #24 is merged, rebase |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one nitpick.
After approval from @InputUsername (ref: #27), I've rebased this branch against main, squashed the commits, and fixed my nitpick. This is just so we can get this great PR into main. |
d06faf2
to
4e79ad9
Compare
@InputUsername We're all good to merge now. cc: @RustyNova016 - you'll need to do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, compiles on my end.
Looks good. @shymega should we wait until after 0.8.0 is released so we can add docs to everything in one go (e.g. for 0.8.1)? |
The current response models lack any MBID mapping information that would appear on the request, making it impossible to determine if the listen is mapped or not. I've added the missing fields, and added documentation comments for all the other fields of the response. Signed-off-by: Dom Rodriguez <[email protected]>
4e79ad9
to
bb1527c
Compare
Wait for v0.8.1. Sorry, didn't see your ping. |
The current response models lack any MBID mapping information that would appear on the request, making it impossible to determine if the listen is mapped or not.
I've added the missing fields, and added documentation comments for all the other fields of the response.