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

Adds the Faker::Sports::Basketball Generator #1537

Merged
merged 1 commit into from
Feb 14, 2019

Conversation

ecbrodie
Copy link
Contributor

@ecbrodie ecbrodie commented Feb 14, 2019

Motivated by my discovery of the Faker::Football generator and the fact that my two favourite hobbies are basketball and programming, I decided to create a generator for basketball. Introducing the Faker::Sports::Basketball generator!

For now, I have added the following method to the generator (of course, this generator should be open for new methods or even new data added to its existing methods):

  • positions: The 5 standard basketball positions
  • teams: The names of the 30 teams in the NBA
  • players: The names of some NBA players. Since this list in real life is huge, I decided to stick to just the participants of the 2019 NBA All Star Game
  • coaches: The names of NBA coaches, current as of PR publication

PR CAVEATS

  • I named my generator Basketball, but it's very possible that NBA is a more appropriate name. Although the NBA is the most recognizable league in the world, there are of course other popular leagues around the globe (NCAA, Euroleague, etc). It's possible that a generator named "Basketball" should also be including data from more leagues than just basketball
  • I introduced a new module called Sports and nested the Basketball generator inside of it. If this is not welcome, I am okay with moving Basketball to the global Faker namespace. I thought this could be useful because I can see how new generators could naturally be grouped into Sports. But, if this move is accepted, then a future PR could move the Football and WorldCup generators into Sports
  • I did not add any files into the unreleased directory. I was not sure if this was necessary. It was definitely not mentioned in the contributing guide.

Happy hooping!

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.

Looks good 👍

You should update our unreleased_README.md and add the Sport namespace there.

Could you please move Faker::Football to Faker::Sports::Football?

@vbrazo
Copy link
Member

vbrazo commented Feb 14, 2019

I think Faker::WorldCup should be moved to the Sport namespace too. I mean Faker::Sports::WorldCup.

@vbrazo
Copy link
Member

vbrazo commented Feb 14, 2019

This namespace discussion started here: #1318

@vbrazo
Copy link
Member

vbrazo commented Feb 14, 2019

I'll do these changes in a separate PR.

@vbrazo vbrazo merged commit 7f75c1b into faker-ruby:master Feb 14, 2019
@ecbrodie
Copy link
Contributor Author

@vbrazo I'm happy to see my contribution merged in :D. Thanks for picking up those extra tasks. I agree that they should be in a separate PR, in order to maintain small PRs.

davidmorton0 pushed a commit to davidmorton0/faker that referenced this pull request Jul 12, 2021
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