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

Support for more collection types #258

Open
hashhar opened this issue Apr 17, 2020 · 9 comments · May be fixed by #259
Open

Support for more collection types #258

hashhar opened this issue Apr 17, 2020 · 9 comments · May be fixed by #259

Comments

@hashhar
Copy link

hashhar commented Apr 17, 2020

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?

@hashhar
Copy link
Author

hashhar commented Apr 17, 2020

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.

@hashhar
Copy link
Author

hashhar commented Apr 17, 2020

@alastair I think I'm going with the approach used in the _do_collection_query and the methods that use it. It'll allow using both named functions and an easy way to add new types of collections.

hashhar added a commit to hashhar/python-musicbrainzngs that referenced this issue Apr 17, 2020
Signed-off-by: Ashhar Hasan <hashhar_dev@outlook.com>
@alastair
Copy link
Owner

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?

@hashhar
Copy link
Author

hashhar commented Apr 17, 2020

I think my idea would've made the interface a little difficult to use. I was thinking of exposing the _do_collection_put and _do_collection_delete methods as public ones and not add the specific functions which I've added (add_areas_to_collection, add_artists_to_collection etc.).

The users of the package could then directly pass in the correct collection_type to the _do_collection_put/remove methods. But that would increase chances of mistakes and would need people to check what all types were available.

In a magical world I'd expect to be able to have the add_type_to_collection and delete_type_from_collection methods be autogenerated for all the possible types.

I'm satisfied with what I've done for now - just need to test this a some more (mainly around release-groups).

@hashhar
Copy link
Author

hashhar commented Apr 24, 2020

@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.

@alastair
Copy link
Owner

alastair commented May 9, 2020

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

@hashhar
Copy link
Author

hashhar commented Jul 21, 2020

@alastair Just wanted to check if you had a chance to look at this.

@alastair
Copy link
Owner

Thanks for your friendly reminders. It's still been quite busy here, but this is on my pymb list.

@hashhar hashhar linked a pull request Jan 22, 2022 that will close this issue
@bartkl
Copy link

bartkl commented Aug 5, 2022

I'm yearning for this as well. Would be great if this could get merged :).
Thanks for the great work!

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 a pull request may close this issue.

3 participants