-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Rename Faker::Show
to Faker::Theater
#2857
Conversation
The Faker::Show class was namespaced under faker/music/show.rb, and conventionally should be subclassed to Faker::Music::Show. However, the intent of the faker is to provide names for musicals and plays, so opted to call the class `Theater` instead.
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.
Looking good! Left some comments ⭐
@@ -0,0 +1,7 @@ | |||
# Faker::Theater |
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 don't really know what's the reason behind this unreleased
folder but we can skip that and place it in the default folder with the others 👍
# frozen_string_literal: true | ||
|
||
module Faker | ||
class Theater < Base |
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.
Have you tried naming this file to lib/faker/theater.rb
? No need to be nested, unless that's necessary.
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.
You're right. Will fix that!
This one hasn't been updated in a while. It's currently being fixed here #2921 |
Motivation / Background
This Pull Request has been created because of naming issues identified here: #2787
This PR replaces the usage of
Faker::Show
to beFaker::Theater
for the following reasons:The file was created under
faker/music/show.rb
but the class definition isFaker::Show
At the very least the class should be name-spaced correctly asFaker::Music::Show
Faker::Show
is ambiguous for its current usage.Faker::Music::Show
would also be ambiguous. The class supports three methods:adult_musical
kids_musical
play
that generate names for musicals and plays which all can be categorized underTheater
.Additional information
Related: #2787
Checklist
Before submitting the PR make sure the following are checked:
[Fix #issue-number]
If you're proposing a new generator: