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

Faker::Games::Dota missing quotes and new heroes #2907

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

arthurka-o
Copy link

Motivation / Background

This Pull Request has been created because I was trying to generate a quote from random Dota 2 heroes.
And encountered that a lot of heroes don't have any quotes for them. That's why I decided to fill missing quotes.

name = Faker::Games::Dota.hero
quote = Faker::Games::Dota.quote(hero: name) # this will trigger an error in a lot of cases

For this PR I only added missing quotes for heroes from A to D. I'm not sure that Faker team will accept those changes, so I decided to stop on this for now. I'll do other heroes quotes after seeing the green light.

And also I added two missing heroes. This is a small change so I decided to put it in this PR.

Additional information

I also added centaur_warrunner quotes by coping centaur quotes. My reasoning is in the commit message.

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug, refactor something, or add a feature.
  • Tests and Rubocop are passing before submitting your proposed changes.

If you're proposing a new generator or locale:

  • Double-check the existing generators documentation to make sure the new generator you want to add doesn't already exist.
  • You've reviewed and followed the Contributing guidelines.

The hero is called `Centaur Warrunner` but in
`quotes` he's called just `centaur`. It causes
problems if users try to generate quote by
`parameterize(separator: '_')` the name.
I left old `centaur` to not brake people's code.
@stefannibrasil
Copy link
Contributor

Thank you! What if we fixed this by making sure that if a hero does not have a quote, we return nil instead of an error?

Additionally, we could remove the default hero as well. Or we can also remove the dependency between hero and quote.

What do you think?

@arthurka-o
Copy link
Author

Thanks for checking my PR!

What if we fixed this by making sure that if a hero does not have a quote, we return nil instead of an error?

Or we can also remove the dependency between hero and quote.

Yes, I can implement that. But I still want to populate quotes for every hero. So I think it's unnecessary to remove dependency between heroes and quotes.

Additionally, we could remove the default hero as well

After populating all quotes, yeah. And also replace this variable with

@heroes = Faker::Base.fetch_all('games.dota.hero').map { |hero| hero.gsub(/\W+/, '_').downcase }

What's the best way for me to proceed with this PR? I don't want to spam contributors notifications, putting more commits in this PR will do exactly that.

My suggestion is

  • Merge this PR if it's good for you
  • Open another one for returning nil instead of an error
  • And the third one will have all remaining quotes

@stefannibrasil
Copy link
Contributor

HI @arthurka-o thank you for you comment! Catching up with the reviews now.

I don't want to spam contributors notifications, putting more commits in this PR will do exactly that.

It doesn't spam us when you push commits to a open PR 👍

And the third one will have all remaining quotes

Are there still more things to add besides the ones being added here? I'd rather remove the heroes that don't have the quotes for now; we also don't want to add every single hero and subsequent quote -- that's too much. We'll probably organize things around and adding more stuff means more things to shuffle later.

This one looks good to me and it can also handle the case when the hero does not have a quote on this one. Up to you if you decide to do it in a follow up PR. Thanks!

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