-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Deprecate HeroesOfTheStorm.class
#2031
Deprecate HeroesOfTheStorm.class
#2031
Conversation
@@ -2,6 +2,6 @@ en: | |||
faker: | |||
heroes_of_the_storm: | |||
battlegrounds: ["Alterac Pass", "Battlefield of Eternity", "Blackheart's Bay", "Braxis Holdout", "Cursed Hollow", "Dragon Shire", "Garden of Terror", "Hanamura Temple", "Haunted Mines", "Infernal Shrines", "Sky Temple", "Tomb of the Spider Queen", "Towers of Doom", "Volskaya Foundry", "Warhead Junction"] | |||
classes: ["Bruiser", "Healer", "Melee Assassin", "Ranged Assassin", "Support", "Tank"] | |||
classe_names: ["Bruiser", "Healer", "Melee Assassin", "Ranged Assassin", "Support", "Tank"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original author of this feature suggested the name specialization
as an alternative:
#2006 (comment)
What do you think about using specialization
and specialization_names
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
class_name
is fine imo as that's closer to the term often used. However, the typo should be fixed first :P
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can take either name, anyway I fixed the obvious typo first 💦
This is great @koic thank you for doing it! 🙏 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@@ -2,6 +2,6 @@ en: | |||
faker: | |||
heroes_of_the_storm: | |||
battlegrounds: ["Alterac Pass", "Battlefield of Eternity", "Blackheart's Bay", "Braxis Holdout", "Cursed Hollow", "Dragon Shire", "Garden of Terror", "Hanamura Temple", "Haunted Mines", "Infernal Shrines", "Sky Temple", "Tomb of the Spider Queen", "Towers of Doom", "Volskaya Foundry", "Warhead Junction"] | |||
classes: ["Bruiser", "Healer", "Melee Assassin", "Ranged Assassin", "Support", "Tank"] | |||
classe_names: ["Bruiser", "Healer", "Melee Assassin", "Ranged Assassin", "Support", "Tank"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
class_name
is fine imo as that's closer to the term often used. However, the typo should be fixed first :P
# | ||
# @return [String] | ||
# | ||
# @example | ||
# Faker::Games::HeroesOfTheStorm.class #=> "Support" | ||
# Faker::Games::HeroesOfTheStorm.class_name #=> "Support" | ||
# | ||
# @faker.version 1.9.2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
version should be changed to next
due to being a "new" generator
Fixes faker-ruby#2006. This PR solves an issue of overriding Ruby's built-in `Object#class` method. I'd like to maintain `HeroesOfTheStorm.class`'s behavior for a period of time. But I chose to make a breaking change because it differs from the expected behavior for built-in `Object#class` method. I think that the warning for `HeroesOfTheStorm.class` will be removed in near future.
1ca4fb7
to
d3e8929
Compare
Thanks! (Merged with failure in the Ruby head tests) |
Fixes #2006.
This PR solves an issue of overriding Ruby's built-in
Object#class
method.I'd like to maintain
HeroesOfTheStorm.class
's behavior for a period of time.But I chose to make a breaking change because it differs from the expected behavior for built-in
Object#class
method.Also, I think that the warning for
HeroesOfTheStorm.class
will be removed in near future.