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

Please use a different name for "class" #2006

Closed
thejonanshow opened this issue May 22, 2020 · 3 comments · Fixed by #2031
Closed

Please use a different name for "class" #2006

thejonanshow opened this issue May 22, 2020 · 3 comments · Fixed by #2031

Comments

@thejonanshow
Copy link
Contributor

So I was investigating some odd behavior in Ruby this evening and tried to find an object in memory based on it's ancestors with something like this:

os = []
ObjectSpace.each_object do |o| 
  os << o if o.class.ancestors.map(&:to_s).include? "PoorLostOrphanObject"
end

Imagine my surprise when I received the following error:

NoMethodError: undefined method `ancestors' for "Melee Assassin":String

Assuming that Ruby had not added some new Melee Assassin object that had escaped my attention I went looking for the culprit:

os = []; ObjectSpace.each_object { |o| os << o unless o.class.respond_to? :ancestors }
=> [Faker::Games::HeroesOfTheStorm]

I think it's fair to say we can all agree taking over a method like :class is pretty suboptimal, so I thought I'd make a PR to fix it, but I realized that someone, somewhere is most definitely using this code, and their tests will break with some equally confusing errors.

Any thoughts on how to go about undoing this one? There may not be any option but to rename this method hero_class or something similar and let their tests break. Maybe @illsism wants to recommend an alternative name?

def class
fetch('heroes_of_the_storm.classes')
end

@illsism
Copy link
Contributor

illsism commented May 22, 2020

Sure. I think specialization would be a better name.

koic added a commit to koic/faker that referenced this issue Jun 4, 2020
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.
@koic
Copy link
Member

koic commented Jun 4, 2020

@thejonanshow Hi, Jonan 👋 Thank you for the feedback. I've opened #2031.

BTW, RubyKaigi 2020 cancellation announced today 😢
https://esa-pages.io/p/sharing/68/posts/1006/b15a58c675f5a69d06e5.html

Anyway, I'm glad to meet you on GitHub :-) Thank you.

@thejonanshow
Copy link
Contributor Author

HI! 👋

I saw the news, it's very sad but I'm sure we'll get to meet our friends by the river again soon.

Thank you for the PR!

koic added a commit to koic/faker that referenced this issue Jun 5, 2020
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.
Zeragamba pushed a commit that referenced this issue Jun 6, 2020
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.

I think that the warning for `HeroesOfTheStorm.class`
will be removed in near future.
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 a pull request may close this issue.

3 participants