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

Add feature to vote on tags (#240) #241

Closed
wants to merge 5 commits into from

Conversation

gorgobacka
Copy link

@gorgobacka gorgobacka commented Oct 31, 2018

Votes can be submitted in the following format:
{entity_id1: {tag1: vote, ...}, ...}

e.g.: {'releaseId': {'tag_1': 'upvote', ...}, ...}

Therefore, I extended the make_tag_request function. The previous functionality is still the same.

I already tested it on test.musicbrainz.org. But I'm not sure how to write a proper test for it.

@gorgobacka
Copy link
Author

This PR would also solve: #196.

@gorgobacka
Copy link
Author

@alastair Do you have any comments on this PR? Do I need to change something before it can be merged?

@gorgobacka
Copy link
Author

Bump

@alastair
Copy link
Owner

Thanks for the contributions, but I'm sorry I really don't have the time to look through this and merge it. If you're able to use your modifications then I encourage you to use them, but I don't want to make any promises on when I can merge this because I don't want to fail on them.

@gorgobacka
Copy link
Author

OK, thanks for your reply. I already used this implementation several times.

@alastair alastair added this to the 0.7 milestone Sep 10, 2019
@gorgobacka
Copy link
Author

Bump

@gorgobacka
Copy link
Author

Was there anything missing for including it in v0.7?

if isinstance(tags, dict):
for tag, vote in tags.items():
usertag_xml = ET.SubElement(taglist, "{%s}user-tag" % NS)
usertag_xml.set('vote', vote)
Copy link
Owner

Choose a reason for hiding this comment

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

we should raise an error if the vote parameter isn't a valid value

Copy link
Author

Choose a reason for hiding this comment

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

I implemented a ValueError

@@ -1259,8 +1259,10 @@ def submit_tags(**kwargs):
"""Submit user tags.
Takes parameters named e.g. 'artist_tags', 'recording_tags', etc.,
and of the form:
{entity_id1: {tag1: vote, ...}, ...}
Copy link
Owner

Choose a reason for hiding this comment

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

The documentation should be updated to say what the value of "vote" can be - "upvote", "downvote", or "withdraw". I wonder if it also makes sense to support alternate values for for this? Integers? 1, 0, -1? Booleans? True, False, None?

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for your comments. I will try to update this branch soon.

And I need to check, if the code still works after all those years.

Copy link
Author

Choose a reason for hiding this comment

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

I improved the documentation now. Is it better now?

For the time being, I would stick with the official values for a vote ("upvote", "downvote", "withdraw"). At the moment, I'm not convinced to include a logic to convert boolean of interger values to this specific strings.

@gorgobacka
Copy link
Author

@alastair It's ready for review.

@gorgobacka
Copy link
Author

Ping :)

@gorgobacka
Copy link
Author

gorgobacka commented Jun 28, 2022

I opened this PR four years ago and got very little feedback. Is there any interest in it, @alastair? If not, please let me know and I can close it.

@gorgobacka
Copy link
Author

I'm very disappointed... I'm closing this PR now.

@gorgobacka gorgobacka closed this Jan 11, 2023
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.

None yet

2 participants