-
Notifications
You must be signed in to change notification settings - Fork 133
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 support for new type of youtube thumbnail and add missing tests f… #192
Add support for new type of youtube thumbnail and add missing tests f… #192
Conversation
…or maxres and large thumbnails to non-youtube providers
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.
Some files could not be reviewed due to errors:
.rubocop.yml: Style/DotPosition has the wrong namespace - should be Layout
.rubocop.yml: Style/DotPosition has the wrong namespace - should be Layout .rubocop.yml: Style/FileName has the wrong namespace - should be Naming .rubocop.yml: Style/PredicateName has the wrong namespace - should be Naming .rubocop.yml: Style/AccessorMethodName has the wrong namespace - should be Naming Error: The `Style/TrailingCommaInLiteral` cop no longer exists. Please use `Style/TrailingCommaInArrayLiteral` and/or `Style/TrailingCommaInHashLiteral` instead. (obsolete configuration found in .rubocop.yml, please update it) obsolete parameter MaxLineLength (for Style/IfUnlessModifier) found in .rubocop.yml `Style/IfUnlessModifier: MaxLineLength` has been removed. Use `Metrics/LineLength: Max` instead
@@ -3,6 +3,7 @@ | |||
[nil, 'AIzaSyA6PYwSr1EnLFUFy1cZDk3Ifb0rxeJaeZ0'].each do |api_key| | |||
describe VideoInfo::Providers::YoutubePlaylist do | |||
before(:each) do | |||
#TODO: I think this is unnecessary. Should be removed? |
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.
@thibaudgg What do you think of this? I'm pretty sure since we pass in the api key we don't need to set it. Since the tests dont run in my repo tho didn't want to remove this since I can't reliable run the whole test suite.
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.
Yeah that's weird, or the [nil, 'AIzaSyA6PYwSr1EnLFUFy1cZDk3Ifb0rxeJaeZ0'].each do |api_key|
loop could be removed as well, doesn't make sense to test twice the same thing. 🤔
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.
Let me know what you'd like! If possible hoping to get this merged sometime soon as I'm hoping to use it for another project. Really appreciate your fast reply and helping me through these changes thus far!
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 would remove [nil, 'AIzaSyA6PYwSr1EnLFUFy1cZDk3Ifb0rxeJaeZ0'].each do |api_key|
, no reason to test two times the same things. 👍
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.
@thibaudgg removed!
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.
Some files could not be reviewed due to errors:
.rubocop.yml: Style/DotPosition has the wrong namespace - should be Layout
.rubocop.yml: Style/DotPosition has the wrong namespace - should be Layout .rubocop.yml: Style/FileName has the wrong namespace - should be Naming .rubocop.yml: Style/PredicateName has the wrong namespace - should be Naming .rubocop.yml: Style/AccessorMethodName has the wrong namespace - should be Naming Error: The `Style/TrailingCommaInLiteral` cop no longer exists. Please use `Style/TrailingCommaInArrayLiteral` and/or `Style/TrailingCommaInHashLiteral` instead. (obsolete configuration found in .rubocop.yml, please update it) obsolete parameter MaxLineLength (for Style/IfUnlessModifier) found in .rubocop.yml `Style/IfUnlessModifier: MaxLineLength` has been removed. Use `Metrics/LineLength: Max` instead
@thibaudgg This is ready for review as well. |
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.
Looks good, I'm also wondering if dropping completely VCR and just use webmock to stub the request with the response body wouldn't be a better solution long term, but that's another topic 😅
@@ -3,6 +3,7 @@ | |||
[nil, 'AIzaSyA6PYwSr1EnLFUFy1cZDk3Ifb0rxeJaeZ0'].each do |api_key| | |||
describe VideoInfo::Providers::YoutubePlaylist do | |||
before(:each) do | |||
#TODO: I think this is unnecessary. Should be removed? |
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.
Yeah that's weird, or the [nil, 'AIzaSyA6PYwSr1EnLFUFy1cZDk3Ifb0rxeJaeZ0'].each do |api_key|
loop could be removed as well, doesn't make sense to test twice the same thing. 🤔
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.
Some files could not be reviewed due to errors:
.rubocop.yml: Style/DotPosition has the wrong namespace - should be Layout
.rubocop.yml: Style/DotPosition has the wrong namespace - should be Layout .rubocop.yml: Style/FileName has the wrong namespace - should be Naming .rubocop.yml: Style/PredicateName has the wrong namespace - should be Naming .rubocop.yml: Style/AccessorMethodName has the wrong namespace - should be Naming Error: The `Style/TrailingCommaInLiteral` cop no longer exists. Please use `Style/TrailingCommaInArrayLiteral` and/or `Style/TrailingCommaInHashLiteral` instead. (obsolete configuration found in .rubocop.yml, please update it) obsolete parameter MaxLineLength (for Style/IfUnlessModifier) found in .rubocop.yml `Style/IfUnlessModifier: MaxLineLength` has been removed. Use `Metrics/LineLength: Max` instead
This addresses #191 (comment)