search

faker-ruby / faker

A library for generating fake data such as names, addresses, and phone numbers.
MIT License
11.28k stars 3.18k forks source link

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

Closed arthurka-o closed 5 months ago

arthurka-o commented 9 months ago

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:

If you're proposing a new generator or locale:

stefannibrasil commented 9 months ago

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 commented 9 months ago

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

stefannibrasil commented 8 months ago

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 :+1:

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!

stefannibrasil commented 6 months ago

hi @arthurka-o just checking in: are you still interested in working on this?

I understand you want to populate quotes for every hero but that's a lot for us maintain. We'd rather remove the heroes that don't have the quotes. Thanks!

arthurka-o commented 6 months ago

hi @stefannibrasil. Sorry, completely forgot about this PR 😅 I'll remove heroes without quotes this week.

arthurka-o commented 6 months ago

@stefannibrasil now thinking more about this, it's a bad idea to remove heroes that don't have quotes. I'm sure most users don't even use quotes from heroes, and having a full list of heroes is more important.

I want to return an empty string if a hero doesn't have quotes. WDYT? Here's a quick implementation that can be done

def quote(hero: 'abaddon')
  fetch("games.dota.#{hero}.quote")
rescue I18n::MissingTranslationData
  ''
end

but then this test won't work. What should we do it with it then?

stefannibrasil commented 6 months ago

it's a bad idea to remove heroes that don't have quotes. I'm sure most users don't even use quotes from heroes

Could you share more how you got to this conclusion?

and having a full list of heroes is more important

I understand, however there are more than 100 characters and we don't need to include all of them, just a sample (around 15 to 20). We don't need to update the generator to go down to 15 or 20, but we can remove the ones that don't have quotes.

I want to return an empty string if a hero doesn't have quotes

Yeah, that's a valid concern. I understand I was the one who suggested returning nil but thinking more about this, from a user's perspective, if I pass a hero that doesn't have a quote, it would be expected to have an error with instructions on how to generate one, so an empty string wouldn't give any feedback on how to fix the error. However, it's a bit weird that the user needs to know which characters have quotes, so not ideal.

That's where I was coming from about removing the characters that don't have quotes. That's a breaking change that we can highlight in the release notes while not adding more stuff. That way we guarantee the characters we have are compatible with the other generators in Faker::Games::Dota.

arthurka-o commented 6 months ago

From my perspective, as a Dota 2 player and developer, I would assume that if a library can generate Hero names, all heroes would be present. But in the other hand, Faker is not a DB for games, it doesn't have to include all heroes. It's here to generate fake data.

In the end, I think it's fair to remove heroes that don't have quotes. I'll finalize this PR hopefully this weekend. I think it should be fine if I leave quotes that I already added. So my last commit should be just removing heroes.

arthurka-o commented 5 months ago

@stefannibrasil should be good to review :+1: