Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Delegate to :class rather than 'self.class' #8777

Merged
merged 1 commit into from

3 participants

@goshakkk

No description provided.

@josevalim
Owner

Are you sure tests pass? I get a syntax error.

@goshakkk

Indeed they do pass. No syntax error.

@pixeltrix
Owner

They should pass as internally delegate transforms them to self.class: https://github.com/rails/rails/blob/master/activesupport/lib/active_support/core_ext/module/delegation.rb#L137-L138

The feature was added in ad5f18e. Whether you want to merge it or not is purely aesthetic.

@josevalim
Owner

@pixeltrix I missed the commit ad5f18e, thanks. I have just tried it locally, but I wasn't on master. Your call. :)

@pixeltrix pixeltrix merged commit f180784 into rails:master
@pixeltrix
Owner

Normally I'm not a fan of purely cosmetic changes due to them polluting the history but I'm feeling generous this morning :smile:

@goshakkk goshakkk deleted the goshakkk:delegate-class branch
@goshakkk

Thanks for merging :smile: Though, I think it's important to keep Rails sources as clean as possible — a lot of people use them to study Rails internals or just get better at Ruby generally, so the usage of as many own features as possible makes the sources feel consistent.

I'm seeing a lot of inconsistency in Rails currently. It's mostly small things, but that's what makes an impression of code quality. Starting from 'self.class vs :class in delegate to different indentation styles around private/protected keywords to use of either &:name or { |u| u.name} to different hash syntaxes. This list is actually even longer.

I think Rails should be example of consistent and elegant Ruby code which others can learn from. That is my point.

@pixeltrix
Owner

I think it's important to keep Rails sources as clean as possible — a lot of people use them to study Rails internals or just get better at Ruby generally, so the usage of as many own features as possible makes the sources feel consistent.

Trust me, there are better resources for getting learning Ruby :smile:

I'd hate to think that people are copying how we do things - Rails has a very special set of requirements that anyone developing Rails applications doesn't need to worry about. For example I know Rails is often criticised for not maintaining backwards compatibility but we do try not break people's apps unnecessarily and this can leave code being quite messy at times.

I'm seeing a lot of inconsistency in Rails currently.

I think that's inevitable given the number of contributors, the fact that we're all volunteers and that it's been around for so long now. A lot of the inconsistency is purely subjective and changes over time - for example your change has only been possible for a couple of months and the code you changed predates then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jan 6, 2013
  1. @goshakkk
This page is out of date. Refresh to see the latest.
View
2  actionpack/lib/abstract_controller/layouts.rb
@@ -209,7 +209,7 @@ module Layouts
_write_layout_method
end
- delegate :_layout_conditions, :to => "self.class"
+ delegate :_layout_conditions, to: :class
module ClassMethods
def inherited(klass) # :nodoc:
View
2  actionpack/lib/action_view/template/resolver.rb
@@ -118,7 +118,7 @@ def find_all(name, prefix=nil, partial=false, details={}, key=nil, locals=[])
private
- delegate :caching?, :to => "self.class"
+ delegate :caching?, to: :class
# This is what child classes implement. No defaults are needed
# because Resolver guarantees that the arguments are present and
View
2  guides/source/active_support_core_extensions.md
@@ -920,7 +920,7 @@ When interpolated into a string, the `:to` option should become an expression th
delegate :logger, to: :Rails
# delegates to the receiver's class
-delegate :table_name, to: 'self.class'
+delegate :table_name, to: :class
```
WARNING: If the `:prefix` option is `true` this is less generic, see below.
View
2  railties/lib/rails/engine.rb
@@ -410,7 +410,7 @@ def find(path)
self.isolated = false
delegate :middleware, :root, :paths, to: :config
- delegate :engine_name, :isolated?, to: "self.class"
+ delegate :engine_name, :isolated?, to: :class
def initialize
@_all_autoload_paths = nil
View
2  railties/lib/rails/railtie.rb
@@ -172,7 +172,7 @@ def generate_railtie_name(class_or_module)
end
end
- delegate :railtie_name, to: "self.class"
+ delegate :railtie_name, to: :class
def config
@config ||= Railtie::Configuration.new
Something went wrong with that request. Please try again.