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 sports to Faker::Sport #2397

Merged
merged 7 commits into from
Aug 5, 2022
Merged

Conversation

matt17r
Copy link
Contributor

@matt17r matt17r commented Oct 4, 2021

No-Story

Description:

Add the ability to produce sports in Faker, useful for dummy data in apps to do with sports or co-curricular activities in schools (my use case).

To keep it globally applicable, it returns a modern Olympic sport (including Paralympics) by default. You can also fetch any of the sub categories individually (or optionally include them in the main method):

  • Summer Olympics
  • Summer Paralympics
  • Winter Olympics
  • Winter Paralympics
  • Ancient Olympics
  • Unusual sports (just for fun)

Possible issues/opportunities:

  • Does this overlap with Faker::Hobby?
  • Do I need to do commit any other files for documentation or does it get generated automatically from the YARD annotations I included?
  • My tests seem a bit week/limited but they're consistent with many other tests. Is there an exemplar for how to do them well or is checking for one or more word characters the best we can do given the output is pseudorandom?
  • Does it make sense to have the categories as separate methods or should there just be a category: argument on a single method? Faker::Sport.winter_olympics_sport or Faker::Sport.sport(category: :winter_olympics)

Copy link
Member

@psibi psibi left a comment

Choose a reason for hiding this comment

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

LGTM. Somehow I missed the MR. Can you rebase it against master ?

Copy link

@saiqulhaq saiqulhaq left a comment

Choose a reason for hiding this comment

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

LGTM
just missing Github action checks

@stefannibrasil stefannibrasil merged commit 73df1cf into faker-ruby:master Aug 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants