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 documentation and update sports #2600

Merged
merged 2 commits into from Oct 31, 2022

Conversation

matt17r
Copy link
Contributor

@matt17r matt17r commented Oct 22, 2022

Summary

Primarily a documentation change, with minor changes to the list of sports (in English):

  • Added links to existing docs (Mountaineering and Volleyball)
  • Added doc and link for Faker::Sports
  • Updated lists of olympic sports based on latest information from the sources listed in lib/locales/en/sport.yml
    • Add "Ski mountaineering"
    • Replace "Football (5-a-side)" with "Blind football"
  • Added a few more unusual sports from the list at https://www.wonderslist.com/top-10-unusual-sports/

Other Information

Finally, if your pull request affects documentation, please follow the Guidelines.

When I first added sports, I added YARD style docs for the methods. As I write this PR I'm wondering if I shouldn't have written these docs manually? Are they supposed to be generated automatically by YARD?

 - Add links to existing docs (Mountaineering and Volleyball)
 - Add docs and link for Faker::Sports
 - Update lists of olympic sports based on latest information
@matt17r matt17r changed the title Add sports documentation Add sports documentation and update sports Oct 22, 2022
@matt17r
Copy link
Contributor Author

matt17r commented Oct 22, 2022

I don't know why that test had an error on Ruby head. It's exactly the same logic as 9 other tests in the same file (and probably 1,000-1,500 other tests in the codebase) that didn't error out. There doesn't seem to be a way for me to re-run the tests so I'm going to close and re-open the PR to see if that runs them again. Sorry for any unwanted notifications.

Update:
That worked... the original run had an unusual error in Ruby head but no problems when re-run. Given the logic is almost identical to a thousand other tests I'm reasonably confident the test isn't flakey.

@matt17r matt17r closed this Oct 22, 2022
@matt17r matt17r reopened this Oct 22, 2022
Copy link
Contributor

@thdaraujo thdaraujo 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 adding docs! just one small fix and it should be good to go!

lib/locales/en/sport.yml Outdated Show resolved Hide resolved
@thdaraujo
Copy link
Contributor

thdaraujo commented Oct 31, 2022

I don't know why that test had an error on Ruby head. It's exactly the same logic as 9 other tests in the same file (and probably 1,000-1,500 other tests in the codebase) that didn't error out. There doesn't seem to be a way for me to re-run the tests so I'm going to close and re-open the PR to see if that runs them again. Sorry for any unwanted notifications.

Update: That worked... the original run had an unusual error in Ruby head but no problems when re-run. Given the logic is almost identical to a thousand other tests I'm reasonably confident the test isn't flakey.

Interesting.

Looks like the problem is that, sometimes, the generator will return an array of sports instead of just one sport. This is a bug.
Here's how to reproduce the error:

1_000.times do |i|
   sport = Faker::Sport.sport(include_ancient: true)
   if !sport.is_a?(String)
     p sport
     raise "sport should be a string: #{sport.inspect}"
   end
end

# ["Boxing", "Chariot racing", "Discus", "Horse racing", "Long jump", "Pankration", "Pentathlon", "Running", "Wrestling"]
# sport should be a string: ["Boxing", "Chariot racing", "Discus", "Horse racing", "Long jump", "Pankration", "Pentathlon", "Running", "Wrestling"] (RuntimeError)

I opened this issue: #2608

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.

None yet

2 participants