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.

Sign up for GitHub

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

Jump to bottom

use Show object for episode funcs #4

Merged
merged 3 commits into from
Dec 25, 2019
Merged

use Show object for episode funcs #4

merged 3 commits into from
Dec 25, 2019

Conversation

SaintWish
Copy link

Made the episode functions use the Show object, instead of Client object.
This also fixes the "cannot use show (type *tvmaze.Show) as type tvmaze.Show in argument to c.GetEpisodes" error.

@SaintWish
use Show object for episode funcs
2fc5864 
Made the episode functions use the Show object, instead of Client object. This also fixes the "cannot use show (type *tvmaze.Show) as type tvmaze.Show in argument to c.GetEpisodes" error.
@SaintWish
forgot to update test
fc88521
@coveralls
Copy link

coveralls commented Dec 24, 2019 •
edited
Loading

Pull Request Test Coverage Report for Build 114

  • 12 of 28 (42.86%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-4.3%) to 63.768%
Changes Missing Coverage Covered Lines Changed/Added Lines %
tvmaze/show.go 6 10 60.0%
tvmaze/episode.go 6 18 33.33%
Totals Coverage Status
Change from base Build 87: -4.3%
Covered Lines: 132
Relevant Lines: 207
💛 - Coveralls

@mrobinsn
Copy link
Owner

Thanks for the contribution!

Looks good, but one note: Could you keep the Client.GetEpisode* methods in there for backward compatibility? They could simply call your new methods on the Show that gets passed in. Otherwise this would have to be a major version bump.

@SaintWish
Copy link
Author

Good point, I'll add those in now.

mrobinsn
mrobinsn approved these changes Dec 25, 2019
View reviewed changes
Copy link
Owner

@mrobinsn mrobinsn left a comment

Choose a reason for hiding this comment

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

LGTM

@mrobinsn mrobinsn self-assigned this Dec 25, 2019
@mrobinsn mrobinsn merged commit 2609a8a into mrobinsn:master Dec 25, 2019
@mrobinsn
Copy link
Owner

mrobinsn commented Dec 25, 2019 •
edited
Loading

Released in v1.2.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mrobinsn mrobinsn mrobinsn approved these changes

Assignees

@mrobinsn mrobinsn

Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@SaintWish @coveralls @mrobinsn