-
Notifications
You must be signed in to change notification settings - Fork 105
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
Support for more collection types #258
Comments
FYI, my work can be followed at https://github.com/hashhar/python-musicbrainzngs/tree/extended-collections but there won't be anything to see there until someone (@alastair ?) can let me know which way to go. As a last resort I may submit a PR with what I think is good and can rework it according to whatever feedback I get. |
@alastair I think I'm going with the approach used in the |
Signed-off-by: Ashhar Hasan <hashhar_dev@outlook.com>
Thanks for the contributions. I'll take a look at this in the coming week! What were your ideas for a less-duplicated version of this code? Do you have an idea about what the interface might look like? |
I think my idea would've made the interface a little difficult to use. I was thinking of exposing the The users of the package could then directly pass in the correct In a magical world I'd expect to be able to have the I'm satisfied with what I've done for now - just need to test this a some more (mainly around |
@alastair just wanted to remind you of this PR. I've tested this with all the new endpoints that I've added and it's working. One concern I had was that right now I'm throwing exceptions when a list longer than 400 ids is passed in which might break existing users. I was thinking to add a loop inside the function to break into chunks of 400 instead of failing the request altogether. Will make the API easier for the clients to consume - with an increased chance of running into rate limiting. |
I've not forgotten this! I'm trying to find some time in the weekends to finish looking at this, but I keep running out of time. The PR looks good in general, so I'll try and get it merged as soon as possible |
@alastair Just wanted to check if you had a chance to look at this. |
Thanks for your friendly reminders. It's still been quite busy here, but this is on my pymb list. |
I'm yearning for this as well. Would be great if this could get merged :). |
I'm working on adding support for more collection types to enable implementing beetbox/beets#3549.
I had a few questions around how should I do this? I see clear opportunity to DRY some things up to reduce code duplication (since all types of collections use the same API) but that would break existing applications which are using the older methods.
One thing I can do is to leave the older methods there but change their implementation to use the more generic methods I wish to add so that both new and old users can co-exist.
What are other people's thoughts?
The text was updated successfully, but these errors were encountered: