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

Move types to the top level ActiveRecord namespace #15370

Merged
merged 1 commit into from
May 28, 2014

Conversation

sgrif
Copy link
Contributor

@sgrif sgrif commented May 27, 2014

ActiveRecord::ConnectionAdapters::Type::Value => ActiveRecord::Type::Value

@@ -2,7 +2,7 @@ module ActiveRecord
module Properties
extend ActiveSupport::Concern

Type = ConnectionAdapters::Type
Type = ActiveRecord::Type
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we still need the alias? I think it will resolve fine

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, otherwise subclasses of ActiveRecord::Base won't resolve the constant properly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should since Type is inside the same namespace. I think it is missing the autoload definition inside ActiveRecord.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Subclasses of ActiveRecord::Base shouldn't inherit constants defined in the ActiveRecord namespace.

module Foo
  OUTER_CONST = "outer"

  module Bar
    MODULE_CONST = "module"
  end

  class Baz
    include Bar

    PARENT_CONST = "parent"
  end
end

class FooBaz < Foo::Baz
  PARENT_CONST
  MODULE_CONST
  OUTER_CONST # => this line errors
end

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right!

@rafaelfranca
Copy link
Member

Just rebase and make sure you add the # :nodoc:

`ActiveRecord::ConnectionAdapters::Type::Value` =>
`ActiveRecord::Type::Value`
@sgrif
Copy link
Contributor Author

sgrif commented May 28, 2014

Done

@sgrif
Copy link
Contributor Author

sgrif commented May 28, 2014

@rafaelfranca The build failure was memcache, not the changes in this PR.

rafaelfranca added a commit that referenced this pull request May 28, 2014
Move types to the top level `ActiveRecord` namespace
@rafaelfranca rafaelfranca merged commit fcf9b71 into rails:master May 28, 2014
@sgrif sgrif deleted the sg-type-namespace branch March 14, 2018 20:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants