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 Faker::TvShows::Futurama #1858

Merged
merged 2 commits into from
Jun 10, 2020
Merged

Add Faker::TvShows::Futurama #1858

merged 2 commits into from
Jun 10, 2020

Conversation

JoeNyland
Copy link
Contributor

Issue#

No-Story

Description:

Adds a generator for Futurama, which is one of my favourite TV shows.

This generator can be used to generate names, quotes and locations from Futurama in demo apps, tests, etc. I believe other Futurama fans would use this in their apps and tests, as well as myself.

Copy link
Member

@connorshea connorshea left a comment

Choose a reason for hiding this comment

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

I'm not sure if we should accept the Hermes class just because I think that's a bit too specific, but other than that this looks good.

@JoeNyland
Copy link
Contributor Author

@connorshea Thanks for taking a look.

I'm not sure if we should accept the Hermes class just because I think that's a bit too specific

That's a fair argument about the Hermes class, but I have seen other somewhat similar things in some other TvShows(e.g. https://github.com/faker-ruby/faker/blob/master/lib/faker/tv_shows/bojack_horseman.rb#L22-L33). My thinking was that Hermes' quotes were their own thing, compared to the rest of the quotes from the show. Another option would be to move them to be ...::Futurama.hermes_quote, but that was how I arrived at namespacing it them off as their own thing.

lib/faker/tv_shows/futurama/hermes.rb Outdated Show resolved Hide resolved
@Zeragamba Zeragamba requested a review from connorshea May 23, 2020 00:37
@Zeragamba Zeragamba merged commit 619ca78 into faker-ruby:master Jun 10, 2020
@Zeragamba
Copy link
Contributor

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants