-
Notifications
You must be signed in to change notification settings - Fork 50
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
Add join attribute to Artist class and #118
Conversation
Besides adding a new field to Artist objects, this PR should fix our cluttered Sphinx doc of models.py once and for all. @prcutler suggestions for improving my wording of the new "join" attribute are very welcome :-) @AnssiAhola In case you know by heart: See the FIXME in my commit. Could an Artist in the artists list of a Track object also contain the "join" information? One issues with documenting via autoattribute (#: comments) I can't figure out or mabe it's even just not possible: How to put links to other objects within these comments. Eg. here (from my previous PR) I tried to mark that I'm talking about another object but it's not a link, just text: https://python3-discogs-client.readthedocs.io/en/latest/discogs_client.html#discogs_client.models.Release.artists |
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.
I think using join is fine - you could use combine, too. I would remove the quotes from join as its the right word.
I'm trying to re-write the second sentence. What does this refer to? The first result?
first of the artists does contain
#: a keyword like
Tried to look into this a bit and I don't think the |
only the first Artist object in the artist list does contain the join keyword. This is the release we are talking about: https://www.discogs.com/release/21846394-Vinylgroover-Feat-MC-Freestyle-Bassface Here you see a json snippet, only the first artist has a populated "join" field/attribute/? (what's the term for that in original json speak? was it object? not sure, anyway, we should talk Python speak in our docs): beetbox/beets#4401 (comment) So in case of an release that has 2 artists as in the example json abopve, this is a list containing 2 Artist objects: https://python3-discogs-client.readthedocs.io/en/latest/discogs_client.html#discogs_client.models.Release.artists |
Thanks for digging in Anssi! I took the example from the beets issue again and had a look in the original api response, looks like that (really just 1 track on that record):
So the track doesn't contain the field at all as you found out but extraartists does, in that case since the MC is not the "first artist" it doesn't have the "Feat" keyword but has a But I guess that's something to think about in another PR, now I'll concentrate on getting the Artist object documented correctly. Thanks again! |
@prcutler what you think about this slightly changed version:
|
I think that's a lot better! A couple small changes to make it easier to read and for translating (removing e.g.)
One question: Should artists_sort be wrapped in `` ? Thanks! |
document it via autodoc autoattribute (#:). This commit also fixes/unclutters all the other (child)classes in models.py by adding empty autoattribute comments.
4ce9791
to
7fead3d
Compare
@prcutler fixed as suggested. Thanks a lot! :-) Please accept the change, I'll merge then. Thanks! |
Done! Looks good and thanks! |
document it via autodoc autoattribute (#:).
Also fixes/unclutters all the other (child)classes in models.py by adding empty autoattribute comments.