-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
[YouTube] Drop support for removed dislikes #7410
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -62,11 +62,14 @@ export default class YouTubeLikes extends YouTubeVideoBase { | |
id, | ||
}) | ||
if (queryParams && typeof queryParams.withDislikes !== 'undefined') { | ||
const likes = `${metric(statistics.likeCount)} 👍` | ||
const dislikes = | ||
statistics.dislikeCount !== undefined | ||
? `${metric(statistics.dislikeCount)} 👎` | ||
: '' | ||
renderedBadge = { | ||
...renderedBadge, | ||
message: `${metric(statistics.likeCount)} 👍 ${metric( | ||
statistics.dislikeCount | ||
)} 👎`, | ||
message: `${likes} ${dislikes}`, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We already have the Perhaps There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
What is your evidence for this? Do you have some documentation to the API or an example target that includes a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Youtube has dropped the dislike count from public view altogether, and that's not something we'll ever be able to see. I think we should actually remove dislikes across the board There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd rather like this PR to only restore YT stats. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have a feature that was based on an attribute in the API response that no longer exists, which in turn has broken badges. I don't understand what you're suggesting be split, could you elaborate? We need a resolution to the actual issue, i.e. our badge needs to account for the upstream change. Splitting that into multiple PRs with ~5 line diffs is unnecessary overhead for us There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Understood. Removed code IMO related to the dislikes. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Awesome thanks so much! Will give this a final pass here shortly |
||
} | ||
} | ||
return renderedBadge | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,7 +22,7 @@ t.create('video vote count') | |
.expectBadge({ | ||
label: 'likes', | ||
message: Joi.string().regex( | ||
/^([1-9][0-9]*[kMGTPEZY]?|[1-9]\.[1-9][kMGTPEZY]) 👍 ([1-9][0-9]*[kMGTPEZY]?|[1-9]\.[1-9][kMGTPEZY]) 👎$/ | ||
/^([1-9][0-9]*[kMGTPEZY]?|[1-9]\.[1-9][kMGTPEZY]) 👍$/ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for looking at the tests but as noted above we need to change the behavior a bit. Additionally, we should instead add a new test for the scenario described in #7373. Given the specific use case of a missing |
||
), | ||
color: 'red', | ||
link: ['https://www.youtube.com/video/pU9Q6oiQNd0'], | ||
|
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.
As far as I know we don't actually need the
favoriteCount
value for any of our badges, so instead of adding what is essentially a duplicate schema forstatistics
, let's instead make thedislikeCount
field optional by changing fromnonNegativeInteger
tooptionalNonNegativeInteger
(which can be imported from the same validators module)