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.

Sign up for GitHub

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

Jump to bottom

Accidentally using :class instead of :class_name in association gives misleading error #19659

Closed
pixeltrix opened this issue Apr 5, 2015 · 8 comments
Closed

Accidentally using :class instead of :class_name in association gives misleading error #19659

pixeltrix opened this issue Apr 5, 2015 · 8 comments
Labels
activerecord

Comments

@pixeltrix
Copy link
Contributor

If you define an association using a custom class and then make the all-too common typo of using :class instead of :class_name then the change made in 1f006cd will give rise to a hard to track down error in raise_on_type_mismatch! because is_a? is called with a string instead of a class.

Since :class option is private and added for supporting HABTM associations on top of HMT then there is a question whether we should change the option name to be something else less prone to error or try to detect the error when building the association.

wdyt? @tenderlove @sgrif @rafaelfranca @carlosantoniodasilva
@carlosantoniodasilva
Copy link
Member

Right, I agree it might be easy for someone use :class instead of :class_name. Maybe we should use a different name like anonymous_class or klass instead?

@rafaelfranca
Copy link
Member

Renaming seems fine.

@sgrif
Copy link
Contributor

sgrif commented Apr 6, 2015

anonymous_class is pretty intention revealing to me. I wouldn't mind _internal_class or something similar, either.

@pascalbetz
Copy link

+1 on anonymous_class

(someone who made this typo as well)

@meinac
Copy link
Contributor

meinac commented Apr 9, 2015

Why nobody don't think about giving meaningful error message instead of renaming the option name. class is really fit for this option name.
If I was a programmer who don't know the option name and want to assign a class to my relation I would try to use class option without looking to the documentation.

@carlosantoniodasilva
Copy link
Member

@meinac the point is that this option is for internal use only, not for people to consume, and we'd have no way to differentiate when it's being used by the framework or by a developer on its own code (that's why they should not - consider this as private, an implementation detail).

pixeltrix added a commit that referenced this issue Apr 21, 2015
@pixeltrix
Rename association option :class to :anonymous_class
1119c89 
In 1f006c an option was added called :class to allow passing anonymous
classes to association definitions. Since using :class instead of
:class_name is a fairly common typo even amongst experienced developers
this can result in hard to debug errors arising in raise_on_type_mismatch?

To fix this we're renaming the option from :class to :anonymous_class as
that is a more correct description of what the option is for. Since this
was an internal, undocumented option there is no need for a deprecation.

Fixes #19659

(cherry picked from commit ac2b7a5)

Conflicts:
	activerecord/CHANGELOG.md
	activerecord/lib/active_record/associations/builder/association.rb
@dimitarvp
Copy link

I'm not a Rails core dev obviously, but I'm just hoping in to tell you that anonymous_class makes no sense at all to me. Similar to what @meinac stated, as a dev who relies on Rails' convention-over-configuration general approach, I'd naively just go for the class option and then scratch my head for 2 mins before ending up right here in this thread.

I simply opted for using the class_name option everywhere. As a guy managing several projects I value the low numbers of times I ask myself "WTF was I thinking when I coded this?" several weeks/months down the line.

BTW, something like association_class would be much more immediately apparent.

@matthewd
Copy link
Member

anonymous_class makes no sense at all to me

That is by design; no-one is supposed to use this internal option.

@willcosgrove willcosgrove mentioned this issue Feb 18, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
activerecord
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
8 participants
@matthewd @pixeltrix @pascalbetz @carlosantoniodasilva @rafaelfranca @dimitarvp @sgrif @meinac