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::Music::Phish #1387

Merged
merged 3 commits into from
Nov 10, 2018
Merged

Conversation

nbolser
Copy link
Contributor

@nbolser nbolser commented Oct 9, 2018

Thank you for maintaining this gem.

Song list taken from Phish Wikipedia page:

List of songs recorded by Phish

@nbolser nbolser changed the title Add Phish Add Faker::Phish Oct 9, 2018
Copy link
Member

@vbrazo vbrazo left a comment

Choose a reason for hiding this comment

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

Thanks for contributing. Overall looks good 👍

Could you wrap this new module in a namespace? It could be Faker::Music::Phish.

@vbrazo vbrazo changed the title Add Faker::Phish Add Faker::Music::Phish Oct 10, 2018
@nbolser
Copy link
Contributor Author

nbolser commented Oct 10, 2018

@vbrazo Sure thing. Would you like me to take a shot at adding other music modules to the namespace (ex. Umphrey's McGhee)? I'll update when I get home from work.

@vbrazo
Copy link
Member

vbrazo commented Oct 10, 2018

If they're new modules of other bands, I'd prefer to have these new modules in other PRs.
Examples: Add Faker::Music::LedZeppelin, Add Faker::Music::BlackSabbath.

The answer is yes. Feel free to contribute.

README.md Outdated
@@ -130,6 +130,7 @@ Contents
- [Faker::OnePiece](doc/one_piece.md)
- [Faker::Overwatch](doc/overwatch.md)
- [Faker::ParksAndRec](doc/parks_and_rec.md)
- [Faker::Phish](doc/phish.md)
Copy link
Member

Choose a reason for hiding this comment

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

We've just changed the README.md and now it should have only objects that are in the v1.9.1. You need to update the unreleased_CONTENT.md instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. Sorry for the delay. I'll update today.

@vbrazo vbrazo merged commit ef3286b into faker-ruby:master Nov 10, 2018
davidmorton0 pushed a commit to davidmorton0/faker that referenced this pull request Jul 12, 2021
* Add Phish

Song list taken from Phish Wikipedia page:

https://en.wikipedia.org/wiki/List_of_songs_recorded_by_Phish

* Update CHANGELOG.md
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.

2 participants