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

More helpful error message when instantiating an abstract class #9474

Merged
merged 1 commit into from Mar 7, 2013

Conversation

@HonoreDB
Copy link
Contributor

HonoreDB commented Feb 27, 2013

Calling a literal ActiveRecord::Base.new raises NoMethodError,
since it ends up calling Class.abstract_class? which does not exist.
Similarly, instantiating an actual abstract class hits the database,
when conventionally it should immediately throw NotImplementedError.

ActiveRecord::Base can't be made abstract without breaking many,
many things, so check for it separately.

@senny
Copy link
Member

senny commented Feb 28, 2013

can you write two test cases to verify that AR::Base cant be instantiated and that an abstract class can't be instantiated?

Also we would need a CHANGELOG entry explaining the change.

@schneems
Copy link
Member

schneems commented Mar 3, 2013

@HonoreDB comment-ors don't get notified when you push, you have to reply to the thread for them to know. @senny care to take a second look?

@senny
senny reviewed Mar 3, 2013
View changes
activerecord/CHANGELOG.md Outdated
@@ -1,5 +1,9 @@
## Rails 4.0.0 (unreleased) ##

* Throw NotImplementedError when trying to instantiate Base or an abstract class.

This comment has been minimized.

@senny

senny Mar 3, 2013 Member

just a nitpick but it would be nice to put the constant name inside .

@senny
senny reviewed Mar 3, 2013
View changes
activerecord/lib/active_record/inheritance.rb Outdated
@@ -15,6 +15,9 @@ module ClassMethods
# and if the inheritance column is attr accessible, it initializes an
# instance of the given subclass instead of the base class
def new(*args, &block)
if abstract_class? || self == Base
raise NotImplementedError, "Abstract classes can't be instantiated."

This comment has been minimized.

@senny

senny Mar 3, 2013 Member

maybe we could include the name of the class, which is being instantiated in the error message. Something like:

ActiveRecord::Base is an abstract class and can not be instantiated.
--
MyAbstractClass is an abstract class and can not be instantiated.

This would give the user immediate context what happened.

This comment has been minimized.

@schneems

schneems Mar 3, 2013 Member

👍 tell a user what they did, and then tell them it was wrong :)

@senny
Copy link
Member

senny commented Mar 3, 2013

added a few minor comments.

@HonoreDB
Copy link
Contributor Author

HonoreDB commented Mar 3, 2013

Thanks, comments addressed in HonoreDB@f128e79.

@senny
senny reviewed Mar 3, 2013
View changes
activerecord/test/cases/base_test.rb Outdated
def test_initialize_abstract_class
assert_raises(NotImplementedError) do
FirstAbstractClass.new
end

This comment has been minimized.

@senny

senny Mar 3, 2013 Member

you should assign the exception to check the message:

e = assert_raises(XX) {}

assert_equals "<expected>", e.message
@senny
senny reviewed Mar 3, 2013
View changes
activerecord/test/cases/base_test.rb Outdated
def test_initialize_base
assert_raises(NotImplementedError) do
ActiveRecord::Base.new
end

This comment has been minimized.

@senny

senny Mar 3, 2013 Member

same as above

@senny
Copy link
Member

senny commented Mar 3, 2013

the PR does no longer cleanly apply to master. Can you push a rebased version?

Calling a literal ActiveRecord::Base.new raises NoMethodError,
since it ends up calling Class.abstract_class? which does not exist.
Similarly, instantiating an actual abstract class hits the database,
when conventionally it should immediately throw NotImplementedError.

ActiveRecord::Base can't be made abstract without breaking many,
many things, so check for it separately.
@HonoreDB
Copy link
Contributor Author

HonoreDB commented Mar 3, 2013

Rebased, tests now check the error message in addition to the type.

@senny
Copy link
Member

senny commented Mar 3, 2013

@HonoreDB amazing! thanks for the fast update. 💛

@carlosantoniodasilva @rafaelfranca can you take a final look?

rafaelfranca added a commit that referenced this pull request Mar 7, 2013
More helpful error message when instantiating an abstract class

Conflicts:
	activerecord/CHANGELOG.md
@rafaelfranca rafaelfranca merged commit 53f18f2 into rails:master Mar 7, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.