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?

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

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!

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!

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

@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?

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.

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.

@stefannibrasil should be good to review 👍