Bunch of patches

3 views
Skip to first unread message

Sven Fuchs

unread,
Nov 21, 2010, 5:13:20 PM11/21/10
to rubyonra...@googlegroups.com
Hey guys, I've pushed a bunch of patches and tickets today:

With I18n 0.5.0 we'll ship some improvements to the exception api. This patch makes the translation view helper leverage those improvements (by not catching exceptions itself any more).

We've shipped a backport of 1.9 string interpolation syntax for quite some versions in I18n. Now we're going to not use String#% anymore but implement our own helper method for it. Meanwhile Rails (and probably others) has started relying on this behavior and it seems appropriate to move this to ActiveSupport (should probably have been done right from the beginning).

This is just a small bug. ActionDispatch::Integration::Runner implements method_missing but misses respond_to?. This can make tests fail where application code relies on respond_to?

Ticket was opened by Luke Melia in August. I've now extracted an implementation from adva-cms2 that solves the problem and also seems to speed up polymorphic_url due to caching. Caching still needs to be tweaked though.

A ticket from the stone age when engines still were a rather obscure thing and people failed to see a usecase. We clearly have a need for this and thus I'm trying to bump this once again. The patch doesn't break bc.

My last tickets were lying around for quite a while so I'm trying to raise some attention over here.

Aaron Patterson

unread,
Nov 22, 2010, 12:02:25 PM11/22/10
to rubyonra...@googlegroups.com

Thanks Sven! I'll look through these today.

--
Aaron Patterson
http://tenderlovemaking.com/

Sven Fuchs

unread,
Nov 22, 2010, 12:53:52 PM11/22/10
to rubyonra...@googlegroups.com
On Nov 22, 2010, at 6:02 PM, Aaron Patterson wrote:
> Thanks Sven! I'll look through these today.

Awesome :)

I think I've added a note in the ticket about which branch I've patched against. I'm usually not quite sure about this. Lemme know if something needs to be rebased or something. (I'm hanging out on irc, too.)

Aaron Patterson

unread,
Nov 23, 2010, 11:55:49 AM11/23/10
to rubyonra...@googlegroups.com
On Sun, Nov 21, 2010 at 11:13:20PM +0100, Sven Fuchs wrote:
> Hey guys, I've pushed a bunch of patches and tickets today:
>
> https://rails.lighthouseapp.com/projects/8994/tickets/5969-bump-i18n-and-make-translationhelper-use-new-rescue_format-option#ticket-5969-8
> https://github.com/svenfuchs/rails/commit/a0f059618cb6eb46dfb97ef49004da776bf00d4c
> With I18n 0.5.0 we'll ship some improvements to the exception api. This patch makes the translation view helper leverage those improvements (by not catching exceptions itself any more).

Can you rebase this one from master? It didn't apply cleanly for me.

> https://rails.lighthouseapp.com/projects/8994/tickets/6013-move-19-string-interpolation-syntax-backport-from-i18n-to-activesupport
> https://github.com/svenfuchs/rails/commit/85a05662fe7e0b06e9c7cd93cae9c3721058d879
> We've shipped a backport of 1.9 string interpolation syntax for quite some versions in I18n. Now we're going to not use String#% anymore but implement our own helper method for it. Meanwhile Rails (and probably others) has started relying on this behavior and it seems appropriate to move this to ActiveSupport (should probably have been done right from the beginning).

I'm still working on this. I'd like to get this patch in the backports
gem. I'm not sure which is more appropriate, AS, or backports:

https://github.com/marcandre/backports

> https://rails.lighthouseapp.com/projects/8994/tickets/6027-define-respond_to-on-actiondispatchintegrationrunner
> https://github.com/svenfuchs/rails/commit/64438c2987b6ead8ec28f5962d7d43fa7f01bc3c
> This is just a small bug. ActionDispatch::Integration::Runner implements method_missing but misses respond_to?. This can make tests fail where application code relies on respond_to?

This is applied.

> https://rails.lighthouseapp.com/projects/8994/tickets/2986-polymorphic_url-should-handle-sti-better#ticket-2986-13
> https://github.com/svenfuchs/rails/commit/2d298f2a140cdc8388bf09ce517dd73b698c7a4e
> Ticket was opened by Luke Melia in August. I've now extracted an implementation from adva-cms2 that solves the problem and also seems to speed up polymorphic_url due to caching. Caching still needs to be tweaked though.
>
> https://rails.lighthouseapp.com/projects/8994/tickets/1975-allow-to-register-javascriptstylesheet_expansions-to-existing-symbols#ticket-1975-15
> https://github.com/svenfuchs/rails/commit/b712476ac3f20d1e976ea431ccf44827c419a281
> A ticket from the stone age when engines still were a rather obscure thing and people failed to see a usecase. We clearly have a need for this and thus I'm trying to bump this once again. The patch doesn't break bc.

I'm still working on these two. :-)

Sven Fuchs

unread,
Nov 23, 2010, 12:12:25 PM11/23/10
to rubyonra...@googlegroups.com
On Nov 23, 2010, at 5:55 PM, Aaron Patterson wrote:
> On Sun, Nov 21, 2010 at 11:13:20PM +0100, Sven Fuchs wrote:
>> https://rails.lighthouseapp.com/projects/8994/tickets/5969-bump-i18n-and-make-translationhelper-use-new-rescue_format-option#ticket-5969-8
>> https://github.com/svenfuchs/rails/commit/a0f059618cb6eb46dfb97ef49004da776bf00d4c
>> With I18n 0.5.0 we'll ship some improvements to the exception api. This patch makes the translation view helper leverage those improvements (by not catching exceptions itself any more).
>
> Can you rebase this one from master? It didn't apply cleanly for me.

Sure. Will do that tomorrow.

>> https://rails.lighthouseapp.com/projects/8994/tickets/6013-move-19-string-interpolation-syntax-backport-from-i18n-to-activesupport
>> https://github.com/svenfuchs/rails/commit/85a05662fe7e0b06e9c7cd93cae9c3721058d879
>> We've shipped a backport of 1.9 string interpolation syntax for quite some versions in I18n. Now we're going to not use String#% anymore but implement our own helper method for it. Meanwhile Rails (and probably others) has started relying on this behavior and it seems appropriate to move this to ActiveSupport (should probably have been done right from the beginning).
>
> I'm still working on this. I'd like to get this patch in the backports
> gem. I'm not sure which is more appropriate, AS, or backports:
>
> https://github.com/marcandre/backports

Oh, I didn't even know about that. Is this required by Rails though? At least ActionPack relies on that String#% patch (routes behavior that uses the %{foo} syntax, iirc) but doesn't seem to require the Backports gem? In that case I'd guess it would need to go into ActiveSupport.

>> https://rails.lighthouseapp.com/projects/8994/tickets/6027-define-respond_to-on-actiondispatchintegrationrunner
>> https://github.com/svenfuchs/rails/commit/64438c2987b6ead8ec28f5962d7d43fa7f01bc3c
>> This is just a small bug. ActionDispatch::Integration::Runner implements method_missing but misses respond_to?. This can make tests fail where application code relies on respond_to?
>
> This is applied.

Thank you :)

>> https://rails.lighthouseapp.com/projects/8994/tickets/2986-polymorphic_url-should-handle-sti-better#ticket-2986-13
>> https://github.com/svenfuchs/rails/commit/2d298f2a140cdc8388bf09ce517dd73b698c7a4e
>> Ticket was opened by Luke Melia in August. I've now extracted an implementation from adva-cms2 that solves the problem and also seems to speed up polymorphic_url due to caching. Caching still needs to be tweaked though.
>>
>> https://rails.lighthouseapp.com/projects/8994/tickets/1975-allow-to-register-javascriptstylesheet_expansions-to-existing-symbols#ticket-1975-15
>> https://github.com/svenfuchs/rails/commit/b712476ac3f20d1e976ea431ccf44827c419a281
>> A ticket from the stone age when engines still were a rather obscure thing and people failed to see a usecase. We clearly have a need for this and thus I'm trying to bump this once again. The patch doesn't break bc.
>
> I'm still working on these two. :-)

Anything I can help with here?

Xavier Noria

unread,
Nov 23, 2010, 12:20:16 PM11/23/10
to rubyonra...@googlegroups.com
On Tue, Nov 23, 2010 at 6:12 PM, Sven Fuchs <sven...@artweb-design.de> wrote:

> Oh, I didn't even know about that. Is this required by Rails though? At least ActionPack relies on that String#% patch (routes behavior that uses the %{foo} syntax, iirc) but doesn't seem to require the Backports gem? In that case I'd guess it would need to go into ActiveSupport.

We've used the backports gem as inspiration or source for some
backports in AS, but it is not a Rails dependency.

Aaron Patterson

unread,
Nov 23, 2010, 12:34:59 PM11/23/10
to rubyonra...@googlegroups.com
On Tue, Nov 23, 2010 at 06:12:25PM +0100, Sven Fuchs wrote:
> On Nov 23, 2010, at 5:55 PM, Aaron Patterson wrote:
> > On Sun, Nov 21, 2010 at 11:13:20PM +0100, Sven Fuchs wrote:
> >> https://rails.lighthouseapp.com/projects/8994/tickets/5969-bump-i18n-and-make-translationhelper-use-new-rescue_format-option#ticket-5969-8
> >> https://github.com/svenfuchs/rails/commit/a0f059618cb6eb46dfb97ef49004da776bf00d4c
> >> With I18n 0.5.0 we'll ship some improvements to the exception api. This patch makes the translation view helper leverage those improvements (by not catching exceptions itself any more).
> >
> > Can you rebase this one from master? It didn't apply cleanly for me.
>
> Sure. Will do that tomorrow.
>
> >> https://rails.lighthouseapp.com/projects/8994/tickets/6013-move-19-string-interpolation-syntax-backport-from-i18n-to-activesupport
> >> https://github.com/svenfuchs/rails/commit/85a05662fe7e0b06e9c7cd93cae9c3721058d879
> >> We've shipped a backport of 1.9 string interpolation syntax for quite some versions in I18n. Now we're going to not use String#% anymore but implement our own helper method for it. Meanwhile Rails (and probably others) has started relying on this behavior and it seems appropriate to move this to ActiveSupport (should probably have been done right from the beginning).
> >
> > I'm still working on this. I'd like to get this patch in the backports
> > gem. I'm not sure which is more appropriate, AS, or backports:
> >
> > https://github.com/marcandre/backports
>
> Oh, I didn't even know about that. Is this required by Rails though? At least ActionPack relies on that String#% patch (routes behavior that uses the %{foo} syntax, iirc) but doesn't seem to require the Backports gem? In that case I'd guess it would need to go into ActiveSupport.

I think if AP was relying on behavior supplied by i18n, we should
continue to use i18n for that support. I mean, we should switch it to
use the new method in i18n.

I'm only worried about end users that inadvertently used this backport.

Aaron Patterson

unread,
Nov 23, 2010, 6:15:19 PM11/23/10
to rubyonra...@googlegroups.com
On Sun, Nov 21, 2010 at 11:13:20PM +0100, Sven Fuchs wrote:
> Hey guys, I've pushed a bunch of patches and tickets today:
>
> https://rails.lighthouseapp.com/projects/8994/tickets/5969-bump-i18n-and-make-translationhelper-use-new-rescue_format-option#ticket-5969-8
> https://github.com/svenfuchs/rails/commit/a0f059618cb6eb46dfb97ef49004da776bf00d4c
> With I18n 0.5.0 we'll ship some improvements to the exception api. This patch makes the translation view helper leverage those improvements (by not catching exceptions itself any more).
>
> https://rails.lighthouseapp.com/projects/8994/tickets/6013-move-19-string-interpolation-syntax-backport-from-i18n-to-activesupport
> https://github.com/svenfuchs/rails/commit/85a05662fe7e0b06e9c7cd93cae9c3721058d879
> We've shipped a backport of 1.9 string interpolation syntax for quite some versions in I18n. Now we're going to not use String#% anymore but implement our own helper method for it. Meanwhile Rails (and probably others) has started relying on this behavior and it seems appropriate to move this to ActiveSupport (should probably have been done right from the beginning).
>
> https://rails.lighthouseapp.com/projects/8994/tickets/6027-define-respond_to-on-actiondispatchintegrationrunner
> https://github.com/svenfuchs/rails/commit/64438c2987b6ead8ec28f5962d7d43fa7f01bc3c
> This is just a small bug. ActionDispatch::Integration::Runner implements method_missing but misses respond_to?. This can make tests fail where application code relies on respond_to?
>
> https://rails.lighthouseapp.com/projects/8994/tickets/2986-polymorphic_url-should-handle-sti-better#ticket-2986-13
> https://github.com/svenfuchs/rails/commit/2d298f2a140cdc8388bf09ce517dd73b698c7a4e
> Ticket was opened by Luke Melia in August. I've now extracted an implementation from adva-cms2 that solves the problem and also seems to speed up polymorphic_url due to caching. Caching still needs to be tweaked though.

This looks good. Can you backport the patch to master too? Then I'll
apply and push both. Thanks!

> https://rails.lighthouseapp.com/projects/8994/tickets/1975-allow-to-register-javascriptstylesheet_expansions-to-existing-symbols#ticket-1975-15
> https://github.com/svenfuchs/rails/commit/b712476ac3f20d1e976ea431ccf44827c419a281
> A ticket from the stone age when engines still were a rather obscure thing and people failed to see a usecase. We clearly have a need for this and thus I'm trying to bump this once again. The patch doesn't break bc.

Can you backport this patch to master?

Sven Fuchs

unread,
Nov 24, 2010, 11:36:31 AM11/24/10
to rubyonra...@googlegroups.com
On Nov 23, 2010, at 5:55 PM, Aaron Patterson wrote:
Can you rebase this one from master?  It didn't apply cleanly for me.

I've rebased all remaining patches to master.

I think these are fine:

Allow registering javascript/stylesheet_expansions to existing symbols

Make TranslationHelper#translate use the :rescue_format option in I18n 0.5.0

Move 1.9 string interpolation syntax backport from I18n to ActiveSupport


This one still needs some thoughts though:

Make polymorphic_url walk up the inheritance chain for given records

See:


As you see I've just commented out these two linked lines. It seems this was introduced in https://github.com/rails/rails/commit/613cbe1f0075eed7ebf1ac521e99f652f6891330 I don't quite get that change, shouldn't self always respond to the generated named_route helper here? And if it doesn't, doesn't that indicate some flaw in a different layer?

Maybe that's a discussion we should take off-list. I'll contact Piotr about this :)


Reply all
Reply to author
Forward
0 new messages