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

Update to the UserListensResponse models to add MBID mapping and documentation #22

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

RustyNova016
Copy link
Contributor

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.

@shymega
Copy link
Collaborator

shymega commented Mar 4, 2024

Would rather not merge this, the history isn't linear. Is it intended for #24 to supersede this PR?

@InputUsername
Copy link
Owner

I agree, seems like a duplicate of #24 but including the added docs?

@RustyNova016
Copy link
Contributor Author

I agree, seems like a duplicate of #24 but including the added docs?

You asked on #20 to separate the docs, so I separated them. #24 can be merged now, but #22 is meant to be for the documentation update.

Should I had waited for #24 to go through?

@InputUsername
Copy link
Owner

I agree, seems like a duplicate of #24 but including the added docs?

You asked on #20 to separate the docs, so I separated them. #24 can be merged now, but #22 is meant to be for the documentation update.

Should I had waited for #24 to go through?

No that's fine, we can merge this one later 😄

@shymega
Copy link
Collaborator

shymega commented Mar 8, 2024

We can merge this once the other PR goes through, but the duplicate commit history should be double-checked before merging.

@RustyNova016
Copy link
Contributor Author

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.

@shymega
Copy link
Collaborator

shymega commented Mar 12, 2024

You can remove the unused commits in a rebase, and once #24 is merged, rebase main onto this branch, and push. I can see how the commits ended up this way here, but it does - and can - mess up history.

Copy link
Collaborator

@shymega shymega left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one nitpick.

.gitignore Outdated Show resolved Hide resolved
@shymega
Copy link
Collaborator

shymega commented Apr 2, 2024

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.

@shymega shymega force-pushed the updates/UserListens branch 2 times, most recently from d06faf2 to 4e79ad9 Compare April 2, 2024 22:50
@shymega
Copy link
Collaborator

shymega commented Apr 2, 2024

@InputUsername We're all good to merge now.

cc: @RustyNova016 - you'll need to do git pull --rebase.

Copy link
Collaborator

@shymega shymega left a 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.

@InputUsername
Copy link
Owner

InputUsername commented Apr 3, 2024

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)?

@InputUsername InputUsername added this to the v0.8.1 milestone Jul 28, 2024
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]>
@shymega
Copy link
Collaborator

shymega commented Jul 30, 2024

Wait for v0.8.1. Sorry, didn't see your ping.

@InputUsername InputUsername modified the milestones: v0.8.1, v0.8.2 Dec 27, 2024
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.

3 participants