The following construct is an ActiveSupport-ism:
returning(Foo.new) do |foo|
...
end
I don''t especially like it, since it''s both more verbose and
less efficient
than the direct alternative:
foo = Foo.new
...
foo
It doesn''t occur many times in Merb, so does anyone agree with me that
it
should be removed?
I tried doing this (patch attached) and I find the code more readable
without. For example, in lib/merb/mixins/responder.rb
Before:
def <=>(entry)
returning((entry.quality <=> quality).to_s) do |c|
c.replace((index <=> entry.index).to_s) if c ==
''0''
end.to_i
end
After:
def <=>(entry)
c = (entry.quality <=> quality).to_s
c.replace((index <=> entry.index).to_s) if c ==
''0''
c.to_i
end
This second form also more clearly begs the question, why is the result from
<=> being converted to string and then back to integer? I don''t
know the
answer to that :-)
I believe the attached patch is OK - it doesn''t break any specs. It
doesn''t
remove the definition of Object#returning itself, or its spec.
Regards,
Brian.
-------------- next part --------------
Index: lib/merb/gems.rb
==================================================================---
lib/merb/gems.rb (revision 508)
+++ lib/merb/gems.rb (working copy)
@@ -27,16 +27,16 @@
end
def list_required(timing = :all)
- returning({}) do |required|
- with_manifest do |manifest|
- manifest.each do |gem,options|
- gem_when = (options["when"] || :after).to_sym
- if timing == :all || gem_when == timing
- required[gem] = options["version"]
- end
+ required = {}
+ with_manifest do |manifest|
+ manifest.each do |gem,options|
+ gem_when = (options["when"] || :after).to_sym
+ if timing == :all || gem_when == timing
+ required[gem] = options["version"]
end
end
end
+ required
end
def load_required(timing = :after)
Index: lib/merb/mixins/responder.rb
==================================================================---
lib/merb/mixins/responder.rb (revision 508)
+++ lib/merb/mixins/responder.rb (working copy)
@@ -65,18 +65,18 @@
def self.parse(accept_header)
# parse the raw accept header into a unique, sorted array of
AcceptType objects
- returning( accept_header.split(/,/).enum_for(:each_with_index).map
do |entry,index|
+ list = accept_header.split(/,/).enum_for(:each_with_index).map do
|entry,index|
AcceptType.new(entry,index += 1)
- end.sort.uniq ) do |list|
- # firefox (and possibly other browsers) send broken default
accept headers.
- # fix them up by sorting alternate xml forms (namely
application/xhtml+xml)
- # ahead of pure xml types (application/xml,text/xml).
- if app_xml = list.detect{|e| e.super_range ==
''application/xml''}
- list.select{|e| e.to_s =~ /\+xml/}.each { |acc_type|
- list[list.index(acc_type)],list[list.index(app_xml)] =
- list[list.index(app_xml)],list[list.index(acc_type)] }
- end
+ end.sort.uniq
+ # firefox (and possibly other browsers) send broken default accept
headers.
+ # fix them up by sorting alternate xml forms (namely
application/xhtml+xml)
+ # ahead of pure xml types (application/xml,text/xml).
+ if app_xml = list.detect{|e| e.super_range ==
''application/xml''}
+ list.select{|e| e.to_s =~ /\+xml/}.each { |acc_type|
+ list[list.index(acc_type)],list[list.index(app_xml)] =
+ list[list.index(app_xml)],list[list.index(acc_type)] }
end
+ list
end
private
@@ -128,9 +128,9 @@
end
def <=>(entry)
- returning((entry.quality <=> quality).to_s) do |c|
- c.replace((index <=> entry.index).to_s) if c ==
''0''
- end.to_i
+ c = (entry.quality <=> quality).to_s
+ c.replace((index <=> entry.index).to_s) if c ==
''0''
+ c.to_i
end
def eql?(entry)
Index: lib/merb/template/erubis.rb
==================================================================---
lib/merb/template/erubis.rb (revision 508)
+++ lib/merb/template/erubis.rb (working copy)
@@ -40,12 +40,12 @@
return @@erbs[path]
else
begin
- returning ::Erubis::MEruby.new(IO.read(path)) do |eruby|
- eruby.init_evaluator :filename => path
- if cache_template?(path)
- @@erbs[path], @@mtimes[path] = eruby, Time.now
- end
- end
+ eruby = ::Erubis::MEruby.new(IO.read(path))
+ eruby.init_evaluator :filename => path
+ if cache_template?(path)
+ @@erbs[path], @@mtimes[path] = eruby, Time.now
+ end
+ eruby
rescue Errno::ENOENT
raise "No template found at path: #{path}"
end
Index: lib/merb/mailer.rb
==================================================================---
lib/merb/mailer.rb (revision 508)
+++ lib/merb/mailer.rb (working copy)
@@ -52,9 +52,9 @@
def initialize(o={})
self.config = :sendmail if config.nil?
o[:rawhtml] = o.delete(:html)
- @mail = returning MailFactory.new() do |m|
- o.each { |k,v| m.send "#{k}=", v }
- end
+ m = MailFactory.new()
+ o.each { |k,v| m.send "#{k}=", v }
+ @mail = m
end
end
On Sep 4, 2007, at 2:46 PM, Brian Candler wrote:> The following construct is an ActiveSupport-ism: > > returning(Foo.new) do |foo| > ... > end > > I don''t especially like it, since it''s both more verbose and less > efficient > than the direct alternative: > > foo = Foo.new > ... > foo >I think it should go too. It doesn''t add much for readability (even though it removes one repeated noun) and seems quite a bit slower. Duane Johnson (canadaduane)
On Sep 4, 2007, at 1:57 PM, Duane Johnson wrote:> > On Sep 4, 2007, at 2:46 PM, Brian Candler wrote: > >> The following construct is an ActiveSupport-ism: >> >> returning(Foo.new) do |foo| >> ... >> end >> >> I don''t especially like it, since it''s both more verbose and less >> efficient >> than the direct alternative: >> >> foo = Foo.new >> ... >> foo >> > > I think it should go too. It doesn''t add much for readability (even > though it removes one repeated noun) and seems quite a bit slower. > > Duane Johnson > (canadaduane) >I''m fine with axing it. Cheers -- Ezra Zygmuntowicz -- Founder & Ruby Hacker -- ez at engineyard.com -- Engine Yard, Serious Rails Hosting -- (866) 518-YARD (9273)
On Tue, Sep 04, 2007 at 02:02:15PM -0700, Ezra Zygmuntowicz wrote:> I''m fine with axing it.Should Merb''s definition of Object#returning also go? Anybody who wants to use ActiveSupport can still require it themselves. Now that Merb is aiming to be ORM-neutral, perhaps extra care needs to be taken with core_ext. Some users won''t have ActiveSupport loaded, but others will (e.g. because they are using ActiveRecord). There''s a risk that Merb''s core extensions may clash with or be incompatible with an ORM''s extensions with the same method names. Hash#to_xml and Hash.from_xml are particular examples that spring to mind. Regards, Brian.
On Sep 4, 2007, at 4:46 PM, Brian Candler wrote:> > Before: > > def <=>(entry) > returning((entry.quality <=> quality).to_s) do |c| > c.replace((index <=> entry.index).to_s) if c == ''0'' > end.to_i > end > > After: > > def <=>(entry) > c = (entry.quality <=> quality).to_s > c.replace((index <=> entry.index).to_s) if c == ''0'' > c.to_i > endAh... That''d be me. I''m still fond of the k-combinator, but not so fond of it that I can''t see a misuse these days :) The .to_s and then subsequent .to_i were merely facilitators to being able to use returning. That snippet would probably be best rewritten as: def <=>(entry) c = entry.quality <=> quality c = index <=> entry.index if c == 0 c end specs still pass with that change made. -- Stephen Caudill (voxdolo)
Fixed this in r511. I still think there''s a time and a place for that pattern. It''s not magical and it follows a clear precedent in good OO design. It is decidedly *not* as performant as it''s sometimes longer handed counterpart, but Ruby is a language in which the value of being succinct is frequently more important than being very fast. I''d rather not sacrifice a bit of beauty in my code for a few milliseconds parsing difference, since most of the overhead occurs in method dispatch. just my 2? -- Stephen Caudill (voxdolo) On Sep 4, 2007, at 4:46 PM, Brian Candler wrote:> The following construct is an ActiveSupport-ism: > > returning(Foo.new) do |foo| > ... > end > > I don''t especially like it, since it''s both more verbose and less > efficient > than the direct alternative: > > foo = Foo.new > ... > foo > > It doesn''t occur many times in Merb, so does anyone agree with me > that it > should be removed? > > I tried doing this (patch attached) and I find the code more readable > without. For example, in lib/merb/mixins/responder.rb > > Before: > > def <=>(entry) > returning((entry.quality <=> quality).to_s) do |c| > c.replace((index <=> entry.index).to_s) if c == ''0'' > end.to_i > end > > After: > > def <=>(entry) > c = (entry.quality <=> quality).to_s > c.replace((index <=> entry.index).to_s) if c == ''0'' > c.to_i > end > > This second form also more clearly begs the question, why is the > result from > <=> being converted to string and then back to integer? I don''t > know the > answer to that :-) > > I believe the attached patch is OK - it doesn''t break any specs. It > doesn''t > remove the definition of Object#returning itself, or its spec. > > Regards, > > Brian. > <merb-returning.diff> > _______________________________________________ > Merb-devel mailing list > Merb-devel at rubyforge.org > http://rubyforge.org/mailman/listinfo/merb-devel
I like having the options of using the syntax sugar (when appropriate) of returning and with_options. I totally agree that the framework can do without due these idioms due to performance requirements but as an app developer I''d like the option where cleanliness/beauty is valued more than a small speedup. Zack On 9/4/07, Stephen Caudill <scaudill at gmail.com> wrote:> Fixed this in r511. > > I still think there''s a time and a place for that pattern. It''s not > magical and it follows a clear precedent in good OO design. It is > decidedly *not* as performant as it''s sometimes longer handed > counterpart, but Ruby is a language in which the value of being > succinct is frequently more important than being very fast. I''d > rather not sacrifice a bit of beauty in my code for a few > milliseconds parsing difference, since most of the overhead occurs in > method dispatch. > > just my 2? > > -- > > Stephen Caudill (voxdolo) > > On Sep 4, 2007, at 4:46 PM, Brian Candler wrote: > > > The following construct is an ActiveSupport-ism: > > > > returning(Foo.new) do |foo| > > ... > > end > > > > I don''t especially like it, since it''s both more verbose and less > > efficient > > than the direct alternative: > > > > foo = Foo.new > > ... > > foo > > > > It doesn''t occur many times in Merb, so does anyone agree with me > > that it > > should be removed? > > > > I tried doing this (patch attached) and I find the code more readable > > without. For example, in lib/merb/mixins/responder.rb > > > > Before: > > > > def <=>(entry) > > returning((entry.quality <=> quality).to_s) do |c| > > c.replace((index <=> entry.index).to_s) if c == ''0'' > > end.to_i > > end > > > > After: > > > > def <=>(entry) > > c = (entry.quality <=> quality).to_s > > c.replace((index <=> entry.index).to_s) if c == ''0'' > > c.to_i > > end > > > > This second form also more clearly begs the question, why is the > > result from > > <=> being converted to string and then back to integer? I don''t > > know the > > answer to that :-) > > > > I believe the attached patch is OK - it doesn''t break any specs. It > > doesn''t > > remove the definition of Object#returning itself, or its spec. > > > > Regards, > > > > Brian. > > <merb-returning.diff> > > _______________________________________________ > > Merb-devel mailing list > > Merb-devel at rubyforge.org > > http://rubyforge.org/mailman/listinfo/merb-devel > > _______________________________________________ > Merb-devel mailing list > Merb-devel at rubyforge.org > http://rubyforge.org/mailman/listinfo/merb-devel >
On Tue, Sep 04, 2007 at 06:37:20PM -0700, Zack Chandler wrote:> I like having the options of using the syntax sugar (when appropriate) > of returning and with_options. I totally agree that the framework can > do without due these idioms due to performance requirements but as an > app developer I''d like the option where cleanliness/beauty is valued > more than a small speedup.I think this should be considered as two aspects: (1) Merb providing these constructs for application writers to use (2) Merb using these constructs itself For case (1), I have no problem with Merb providing an ActiveSupport-like feature bundle for users. My concern is whether Merb''s extensions to Object, Hash etc would clash with similar extensions, like ActiveSupport. For case (2), this problem becomes more acute, since (for example) Merb may rely on a particular behaviour of Hash.from_xml, but ActiveSupport may implement Hash.from_xml differently, and itself rely on this different behaviour. It gets even more acute with people running Merb and Rails side-by-side in the same process. To solve this, I am pondering along the following lines: * Merb implements all these extension methods in a distinct namespace, e.g. Hash.merb_from_xml (or perhaps, for class methods, Merb::Hash.from_xml) * Merb''s internal use of these constructs uses these distinct names * If a user ''require''s a specific library, then these get aliased to the expected names, e.g. Hash.from_xml This means that: - Merb and language extensions (e.g. ActiveSupport/Facets/other ORM/other extensions) can be loaded simultaneously - Merb won''t affect the behaviour of the extension library, and vice versa - If a programmer wants to use Merb''s implementation of these language extensions, they are available given a ''require'' Thoughts? Brian.
On Wed, Sep 05, 2007 at 06:35:25AM +0100, Brian Candler wrote:> To solve this, I am pondering along the following lines: > > * Merb implements all these extension methods in a distinct namespace, > e.g. Hash.merb_from_xml (or perhaps, for class methods, Merb::Hash.from_xml) > > * Merb''s internal use of these constructs uses these distinct names > > * If a user ''require''s a specific library, then these get aliased to the > expected names, e.g. Hash.from_xmlI just noticed another mail saying there are now "supported plugins" in the tree. So I propose: 1. Move core_ext into a supported plugin. Where Merb itself currently makes use of features within core_ext: 2. If it can be changed to use native Ruby instead, without making the code harder to maintain or significantly slower, then do that. 3. Otherwise, put the method in merb''s own core_ext but with a private name, e.g. "merb_foo". The public core_ext will alias this to a more common name, e.g. "foo" The core_ext plugin needs to have its own separate specs, so that Merb''s own specs prove that it works without the core_ext plugin. This gives users of Merb three options: - don''t use core extensions - use Merb''s core extensions plugin - use someone else''s core extensions (e.g. ActiveSupport), which may be necessary in the case where an ORM depends on it (e.g. ActiveRecord) Comments? Brian. P.S. I think the plugins directory should have a trunk/tags/branches structure, so that when merb-x.x.x is released, a matching set of plugins can also be tagged. Users can always choose to install a more recent version of the plugin, but this is at their own risk.
On Sep 5, 2007, at 3:36 AM, Brian Candler wrote:> On Wed, Sep 05, 2007 at 06:35:25AM +0100, Brian Candler wrote: >> To solve this, I am pondering along the following lines: >> >> * Merb implements all these extension methods in a distinct >> namespace, >> e.g. Hash.merb_from_xml (or perhaps, for class methods, >> Merb::Hash.from_xml) >> >> * Merb''s internal use of these constructs uses these distinct names >> >> * If a user ''require''s a specific library, then these get aliased >> to the >> expected names, e.g. Hash.from_xml > > I just noticed another mail saying there are now "supported > plugins" in the > tree. > > So I propose: > > 1. Move core_ext into a supported plugin. > > Where Merb itself currently makes use of features within core_ext: > > 2. If it can be changed to use native Ruby instead, without making > the code > harder to maintain or significantly slower, then do that. > > 3. Otherwise, put the method in merb''s own core_ext but with a > private name, > e.g. "merb_foo". The public core_ext will alias this to a more > common > name, e.g. "foo" > > The core_ext plugin needs to have its own separate specs, so that > Merb''s own > specs prove that it works without the core_ext plugin. > > This gives users of Merb three options: > - don''t use core extensions > - use Merb''s core extensions plugin > - use someone else''s core extensions (e.g. ActiveSupport), which > may be > necessary in the case where an ORM depends on it (e.g. ActiveRecord) > > Comments?I think this is extreme. I appreciate the sentiment, but as a committer, I''d still like to have a library at hand to provide conveniences. I''m not opposed to that library living in an insular namespace> P.S. I think the plugins directory should have a trunk/tags/branches > structure, so that when merb-x.x.x is released, a matching set of > plugins > can also be tagged. Users can always choose to install a more > recent version > of the plugin, but this is at their own risk.Sounds like a good idea.
On Sep 5, 2007, at 3:36 AM, Brian Candler wrote:> This gives users of Merb three options: > - don''t use core extensions > - use Merb''s core extensions plugin > - use someone else''s core extensions (e.g. ActiveSupport), which > may be > necessary in the case where an ORM depends on it (e.g. ActiveRecord) > > Comments?Bah.. got trigger happy with the send button. Just to complete my thought: I''m not opposed to that library living in an insular namespace for internal use so it wouldn''t collide with ActiveSupport or Facets. But when it gets down to brass tacks, as a developer of merb, I''d still like to have our own internal lib of methods I can rely on to behave in a certain manner. -- Stephen Caudill (voxdolo)
returning() as the k-combinator is fairly cool, but I like the more object-oriented Object#tap better, and it''s going to be standard in Ruby 1.9 too. See http://eigenclass.org/hiki.rb?Changes+in+Ruby+1.9#l25 I''ve read different things about the overhead of returning()/tap, ranging from mildly more overhead to significant, but haven''t done any measurements myself. But either way, we can make it kick butt in Rubinius, so you should be able to use it guilt-free someday. --josh On Sep 4, 2007, at 6:37 PM, Zack Chandler wrote:> I like having the options of using the syntax sugar (when appropriate) > of returning and with_options. I totally agree that the framework can > do without due these idioms due to performance requirements but as an > app developer I''d like the option where cleanliness/beauty is valued > more than a small speedup. > > Zack > > On 9/4/07, Stephen Caudill <scaudill at gmail.com> wrote: >> Fixed this in r511. >> >> I still think there''s a time and a place for that pattern. It''s not >> magical and it follows a clear precedent in good OO design. It is >> decidedly *not* as performant as it''s sometimes longer handed >> counterpart, but Ruby is a language in which the value of being >> succinct is frequently more important than being very fast. I''d >> rather not sacrifice a bit of beauty in my code for a few >> milliseconds parsing difference, since most of the overhead occurs in >> method dispatch. >> >> just my 2? >> >> -- >> >> Stephen Caudill (voxdolo) >> >> On Sep 4, 2007, at 4:46 PM, Brian Candler wrote: >> >>> The following construct is an ActiveSupport-ism: >>> >>> returning(Foo.new) do |foo| >>> ... >>> end >>> >>> I don''t especially like it, since it''s both more verbose and less >>> efficient >>> than the direct alternative: >>> >>> foo = Foo.new >>> ... >>> foo >>> >>> It doesn''t occur many times in Merb, so does anyone agree with me >>> that it >>> should be removed? >>> >>> I tried doing this (patch attached) and I find the code more >>> readable >>> without. For example, in lib/merb/mixins/responder.rb >>> >>> Before: >>> >>> def <=>(entry) >>> returning((entry.quality <=> quality).to_s) do |c| >>> c.replace((index <=> entry.index).to_s) if c == ''0'' >>> end.to_i >>> end >>> >>> After: >>> >>> def <=>(entry) >>> c = (entry.quality <=> quality).to_s >>> c.replace((index <=> entry.index).to_s) if c == ''0'' >>> c.to_i >>> end >>> >>> This second form also more clearly begs the question, why is the >>> result from >>> <=> being converted to string and then back to integer? I don''t >>> know the >>> answer to that :-) >>> >>> I believe the attached patch is OK - it doesn''t break any specs. It >>> doesn''t >>> remove the definition of Object#returning itself, or its spec. >>> >>> Regards, >>> >>> Brian.-- Josh Susser http://blog.hasmanythrough.com