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

Implemented GET /1/stats/release-group/(release_group_mbid)/listeners #23

Conversation

RustyNova016
Copy link
Contributor

Recreation of #20 due to accidental commits

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.

Only one nitpick. Otherwise fine.

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

shymega commented Mar 4, 2024

Finally, commits need to be documented a bit more. Currently, they're just one-liners with no justification or detail. The PR desc could be improved too.

@shymega shymega requested a review from InputUsername March 4, 2024 18:30
InputUsername
InputUsername previously approved these changes Mar 4, 2024
Copy link
Owner

@InputUsername InputUsername left a comment

Choose a reason for hiding this comment

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

LGTM!

@RustyNova016
Copy link
Contributor Author

And fixed the .gitignore.

InputUsername
InputUsername previously approved these changes Mar 6, 2024
@InputUsername InputUsername requested a review from shymega March 6, 2024 21:37
@shymega
Copy link
Collaborator

shymega commented Mar 8, 2024

Commits should be squashed into one. Same as #24:

I would rather the last three commits are squashed into one, rebased against main, and then reworded to describe the changes.
Currently, it'll lead to a somewhat confusing commit history if we merge as-is.
Sorry, I'm not trying to hold up the PR - just trying to keep a easy to understand commit 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.

Commits to be squashed. See here.

@RustyNova016 RustyNova016 force-pushed the feature/release-group-listeners-endpoint/upstream-merge branch 2 times, most recently from 14e80cb to d897d22 Compare March 8, 2024 08:52
@RustyNova016
Copy link
Contributor Author

This should be fine. It's the first time I did a Rebase and Squash like this so I hope I didn't mess up anything

@InputUsername InputUsername requested a review from shymega March 8, 2024 23:03
@shymega
Copy link
Collaborator

shymega commented Mar 12, 2024

Rebase is fine.

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.

Other than what I've commented on, it's okay. Ideally I'd like more descriptive commit messages, but I guess it's not too importrant.

.gitignore Outdated Show resolved Hide resolved
Added stats_release_group_listeners() on the raw client.
Added the endpoint information to the enum.
Added StatsReleaseGroupListenersResponse, StatsReleaseGroupListenersPayload and StatsReleaseGroupListenersListeners as responce models
@RustyNova016 RustyNova016 force-pushed the feature/release-group-listeners-endpoint/upstream-merge branch from d897d22 to 63a1b0c Compare March 16, 2024 23:05
@RustyNova016
Copy link
Contributor Author

Remove the .gitiignore change

@InputUsername InputUsername requested a review from shymega March 25, 2024 22:08
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, thanks for the PR - and putting up with my nitpicks!

@InputUsername InputUsername merged commit 77e01fe into InputUsername:main Apr 2, 2024
1 check passed
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