Partials, locals, and instance variable propagation
84 views
Skip to first unread message
Andrew Geweke
Nov 22, 2009, 4:03:38 PM11/22/09
to ere...@googlegroups.com
(This is the second of two interesting problems I've run into when
integrating Erector into our large, existing ERb-based Rails app --
see my previous message for more info...)
We're also trying to enable conversion of partials to Erector,
piecemeal -- i.e., you can make a partial use Erector while letting
the calling view stay in ERb. As far as I can tell, right now Erector
passes all controller instance variables to the partial unilaterally,
and omits locals -- 'quux' is unavailable to the widget
Views::Foo::Bar::Baz when called like so:
<%= render :partial => 'foo/bar/baz', :locals => { :quux => 12345 } %>
This has two issues:
1. Partials can't use "needs" in any reasonable fashion, because
they get passed all of the controller's instance variables and will
blow up when extras come through;
2. Partials don't get locals at all.
So I added the following behavior:
A. If partials have a 'needs' clause and are rendered via
"render :partial", they silently ignore extra controller variables;
B. Locals are passed through to partials;
C. Locals override controller instance variables with the same name
(*)
(*) ERb has two namespaces for variables in partials: controller
variables ('@foo') and local variables ('foo'). Erector only has one;
IMHO this is much nicer. But if there's a collision you have to pick,
and this seemed clearly the right choice.
This helped a whole lot. However, there's one remaining issue of
hygiene: partials can still have access to any controller instance
variables they want; there's still this global namespace sitting
around, waiting for dependency-at-a-(long)-distance to be added
accidentally. So I added a single additional Erector::Widget class
method: controller_assigns_propagate_to_partials=. Iff you set this to
false (it's true by default), then "render :partial" *only* passes
through variables specified in :locals; controller variables are
ignored entirely. It's inherited, so you can set it on branches of
your view hierarchy easily and it "just works". It only affects the
"render :partial" case; direct widget instantiation, Erector's
'widget' command, and everything else are not affected.
This lets you choose whether to allow the messiness of letting
controller instance variables propagate down arbitrarily to partials
(probably more compatible with existing apps) or to choose a more
maintainable, hygienic route, and do it on arbitrary swaths of your
view hierarchy.
Same deal as the last message -- please let me know if any of this
seems crazy, or I'm missing something totally obvious that handles
this problem better. I'm trying to solve these problems in a
compatible, non-intrusive manner, but one that also helps Erector
evolve towards fitting in more and more smoothly with existing, large-
scale Rails apps that weren't built with Erector in mind (and which
were built with less-than-perfect discipline...which we all have ;).
I'll happily work towards making this as clean and good as possible.
Thanks!
Andrew
integrating Erector into our large, existing ERb-based Rails app --
see my previous message for more info...)
We're also trying to enable conversion of partials to Erector,
piecemeal -- i.e., you can make a partial use Erector while letting
the calling view stay in ERb. As far as I can tell, right now Erector
passes all controller instance variables to the partial unilaterally,
and omits locals -- 'quux' is unavailable to the widget
Views::Foo::Bar::Baz when called like so:
<%= render :partial => 'foo/bar/baz', :locals => { :quux => 12345 } %>
This has two issues:
1. Partials can't use "needs" in any reasonable fashion, because
they get passed all of the controller's instance variables and will
blow up when extras come through;
2. Partials don't get locals at all.
So I added the following behavior:
A. If partials have a 'needs' clause and are rendered via
"render :partial", they silently ignore extra controller variables;
B. Locals are passed through to partials;
C. Locals override controller instance variables with the same name
(*)
(*) ERb has two namespaces for variables in partials: controller
variables ('@foo') and local variables ('foo'). Erector only has one;
IMHO this is much nicer. But if there's a collision you have to pick,
and this seemed clearly the right choice.
This helped a whole lot. However, there's one remaining issue of
hygiene: partials can still have access to any controller instance
variables they want; there's still this global namespace sitting
around, waiting for dependency-at-a-(long)-distance to be added
accidentally. So I added a single additional Erector::Widget class
method: controller_assigns_propagate_to_partials=. Iff you set this to
false (it's true by default), then "render :partial" *only* passes
through variables specified in :locals; controller variables are
ignored entirely. It's inherited, so you can set it on branches of
your view hierarchy easily and it "just works". It only affects the
"render :partial" case; direct widget instantiation, Erector's
'widget' command, and everything else are not affected.
This lets you choose whether to allow the messiness of letting
controller instance variables propagate down arbitrarily to partials
(probably more compatible with existing apps) or to choose a more
maintainable, hygienic route, and do it on arbitrary swaths of your
view hierarchy.
Same deal as the last message -- please let me know if any of this
seems crazy, or I'm missing something totally obvious that handles
this problem better. I'm trying to solve these problems in a
compatible, non-intrusive manner, but one that also helps Erector
evolve towards fitting in more and more smoothly with existing, large-
scale Rails apps that weren't built with Erector in mind (and which
were built with less-than-perfect discipline...which we all have ;).
I'll happily work towards making this as clean and good as possible.
Thanks!
Andrew
John Firebaugh
Nov 23, 2009, 12:43:12 PM11/23/09
to age...@gmail.com, erector
Just realized I didn't send my last message to the whole list. And now
that I've read Andrew's other message, I'll use this opportunity to
send a more informed reply!
On Mon, Nov 23, 2009 at 9:30 AM, John Firebaugh
<john.fi...@gmail.com> wrote:
> locals off somewhere other than instance variables and autogenerate an
> attr_accessor for each of them. Since the syntax for local variables
> is the same as local methods (providing you don't use a 'self'
> prefix), this would give you essentially the same dual namespaces as
> the ERb partial. But I think I agree that having a single namespace is
> nicer.
> problematic even in the non-partial case. I think it might make sense
> to use this to replace my NON_NEEDED_CONTROLLER_INSTANCE_VARIABLES
> hack and have it work for direct widget instantiation and the 'widget'
> command as well. I.e. if it's set, controller assigns propagate only
> if they are needed or passed explicitly via :locals.
It looks like this is exactly what your
ignore_extra_controller_assigns= option does -- in which case +1, I
wholeheartedly support it. I would just drop
NON_NEEDED_CONTROLLER_INSTANCE_VARIABLES, I think this can replace it
entirely.
John
that I've read Andrew's other message, I'll use this opportunity to
send a more informed reply!
On Mon, Nov 23, 2009 at 9:30 AM, John Firebaugh
<john.fi...@gmail.com> wrote:
> On Sun, Nov 22, 2009 at 1:03 PM, Andrew Geweke <age...@gmail.com> wrote:
>> So I added the following behavior:
>>
>> A. If partials have a 'needs' clause and are rendered via
>> "render :partial", they silently ignore extra controller variables;
>> B. Locals are passed through to partials;
>> C. Locals override controller instance variables with the same name
>> (*)
>>
>> (*) ERb has two namespaces for variables in partials: controller
>> variables ('@foo') and local variables ('foo'). Erector only has one;
>> IMHO this is much nicer. But if there's a collision you have to pick,
>> and this seemed clearly the right choice.
>
> This sounds workable to me. Another option would be to store the
>> So I added the following behavior:
>>
>> A. If partials have a 'needs' clause and are rendered via
>> "render :partial", they silently ignore extra controller variables;
>> B. Locals are passed through to partials;
>> C. Locals override controller instance variables with the same name
>> (*)
>>
>> (*) ERb has two namespaces for variables in partials: controller
>> variables ('@foo') and local variables ('foo'). Erector only has one;
>> IMHO this is much nicer. But if there's a collision you have to pick,
>> and this seemed clearly the right choice.
>
> locals off somewhere other than instance variables and autogenerate an
> attr_accessor for each of them. Since the syntax for local variables
> is the same as local methods (providing you don't use a 'self'
> prefix), this would give you essentially the same dual namespaces as
> the ERb partial. But I think I agree that having a single namespace is
> nicer.
>
>> This helped a whole lot. However, there's one remaining issue of
>> hygiene: partials can still have access to any controller instance
>> variables they want; there's still this global namespace sitting
>> around, waiting for dependency-at-a-(long)-distance to be added
>> accidentally. So I added a single additional Erector::Widget class
>> method: controller_assigns_propagate_to_partials=. Iff you set this to
>> false (it's true by default), then "render :partial" *only* passes
>> through variables specified in :locals; controller variables are
>> ignored entirely. It's inherited, so you can set it on branches of
>> your view hierarchy easily and it "just works". It only affects the
>> "render :partial" case; direct widget instantiation, Erector's
>> 'widget' command, and everything else are not affected.
>
> I've been finding that autopropagation of controller assigns is
>> This helped a whole lot. However, there's one remaining issue of
>> hygiene: partials can still have access to any controller instance
>> variables they want; there's still this global namespace sitting
>> around, waiting for dependency-at-a-(long)-distance to be added
>> accidentally. So I added a single additional Erector::Widget class
>> method: controller_assigns_propagate_to_partials=. Iff you set this to
>> false (it's true by default), then "render :partial" *only* passes
>> through variables specified in :locals; controller variables are
>> ignored entirely. It's inherited, so you can set it on branches of
>> your view hierarchy easily and it "just works". It only affects the
>> "render :partial" case; direct widget instantiation, Erector's
>> 'widget' command, and everything else are not affected.
>
> problematic even in the non-partial case. I think it might make sense
> to use this to replace my NON_NEEDED_CONTROLLER_INSTANCE_VARIABLES
> hack and have it work for direct widget instantiation and the 'widget'
> command as well. I.e. if it's set, controller assigns propagate only
> if they are needed or passed explicitly via :locals.
It looks like this is exactly what your
ignore_extra_controller_assigns= option does -- in which case +1, I
wholeheartedly support it. I would just drop
NON_NEEDED_CONTROLLER_INSTANCE_VARIABLES, I think this can replace it
entirely.
John
Andrew Geweke
Nov 23, 2009, 5:45:26 PM11/23/09
to erector
On Nov 23, 2009, at 9:43 AM, John Firebaugh wrote:
>> I've been finding that autopropagation of controller assigns is
>> problematic even in the non-partial case. I think it might make sense
>> to use this to replace my NON_NEEDED_CONTROLLER_INSTANCE_VARIABLES
>> hack and have it work for direct widget instantiation and the
>> 'widget'
>> command as well. I.e. if it's set, controller assigns propagate only
>> if they are needed or passed explicitly via :locals.
>
> It looks like this is exactly what your
> ignore_extra_controller_assigns= option does -- in which case +1, I
> wholeheartedly support it. I would just drop
> NON_NEEDED_CONTROLLER_INSTANCE_VARIABLES, I think this can replace it
> entirely.
Thanks for the support, Alex and John!
>> I've been finding that autopropagation of controller assigns is
>> problematic even in the non-partial case. I think it might make sense
>> to use this to replace my NON_NEEDED_CONTROLLER_INSTANCE_VARIABLES
>> hack and have it work for direct widget instantiation and the
>> 'widget'
>> command as well. I.e. if it's set, controller assigns propagate only
>> if they are needed or passed explicitly via :locals.
>
> It looks like this is exactly what your
> ignore_extra_controller_assigns= option does -- in which case +1, I
> wholeheartedly support it. I would just drop
> NON_NEEDED_CONTROLLER_INSTANCE_VARIABLES, I think this can replace it
> entirely.
NON_NEEDED_CONTROLLER_INSTANCE_VARIABLES is still needed, as far as I
can tell, in the case where you don't set
"Widget.ignore_extra_controller_assigns = true". Do you think we
should just make that always true for Erector when it's running as
part of Rails, and leave out the configurability? It would make things
simpler, to be sure; I just wanted to be as conservative as possible
with my original patch.
Alex: re: the naming issue -- I agree; the only issue with
"allow_undeclared_parameters" is that it sounds like it should apply
everywhere, and my patch only applies in the Rails case, and only to
the immediate view, not partials. (I really like it that Erector blows
up if I pass extra stuff to my widget, in general, just like Ruby
does...it's just this one case where it has issues.) I'd hate to see
someone say "Erector::Widget.allow_undeclared_parameters = true" and
then go on a wild-goose-chase for hours trying to figure out why it's
not doing what it claims. But I'm all for better names...just having
trouble coming up with one that's specific but less verbose. If we can
brainstorm one, I'll change it right away.
I also just added another commit (http://github.com/ageweke/erector/commit/1cb39a3eb0af22adcd0b2a117d3a40c38afb4052
) that makes Erector work with the Cells plugin -- it doesn't by
default. I totally understand if we don't want that upstream; it's
safe and generic, but specific to this one interaction.
Assuming you're happy with them, what's the next step to getting these
upstream? I'm a Github newbie (the shock!) but am happy to do whatever
makes things easiest for you, too.
Thanks!
Andrew
John Firebaugh
Nov 23, 2009, 6:28:11 PM11/23/09
to age...@gmail.com, erector
On Mon, Nov 23, 2009 at 2:45 PM, Andrew Geweke <age...@gmail.com> wrote:
> On Nov 23, 2009, at 9:43 AM, John Firebaugh wrote:
> Thanks for the support, Alex and John!
>
> NON_NEEDED_CONTROLLER_INSTANCE_VARIABLES is still needed, as far as I
> can tell, in the case where you don't set
> "Widget.ignore_extra_controller_assigns = true". Do you think we
> should just make that always true for Erector when it's running as
> part of Rails, and leave out the configurability? It would make things
> simpler, to be sure; I just wanted to be as conservative as possible
> with my original patch.
I think ignore_extra_controller_assigns should default to true for any
> On Nov 23, 2009, at 9:43 AM, John Firebaugh wrote:
> Thanks for the support, Alex and John!
>
> NON_NEEDED_CONTROLLER_INSTANCE_VARIABLES is still needed, as far as I
> can tell, in the case where you don't set
> "Widget.ignore_extra_controller_assigns = true". Do you think we
> should just make that always true for Erector when it's running as
> part of Rails, and leave out the configurability? It would make things
> simpler, to be sure; I just wanted to be as conservative as possible
> with my original patch.
widget that has needs, and NON_NEEDED_CONTROLLER_INSTANCE_VARIABLES
should be removed. It was created for the same reason as
ignore_extra_controller_assigns -- the presence of stray instance
variables that cause problems once you start using needs.
The ignore_extra_controller_assigns getter and setter are
Rails-specific and should be in rails_widget.rb, no?
Andrew Geweke
Nov 23, 2009, 6:39:54 PM11/23/09
to John Firebaugh, erector
On Nov 23, 2009, at 3:28 PM, John Firebaugh wrote:
> I think ignore_extra_controller_assigns should default to true for any
> widget that has needs, and NON_NEEDED_CONTROLLER_INSTANCE_VARIABLES
> should be removed. It was created for the same reason as
> ignore_extra_controller_assigns -- the presence of stray instance
> variables that cause problems once you start using needs.
Totally agreed. I'll make it that way.
> I think ignore_extra_controller_assigns should default to true for any
> widget that has needs, and NON_NEEDED_CONTROLLER_INSTANCE_VARIABLES
> should be removed. It was created for the same reason as
> ignore_extra_controller_assigns -- the presence of stray instance
> variables that cause problems once you start using needs.
> The ignore_extra_controller_assigns getter and setter are
> Rails-specific and should be in rails_widget.rb, no?
right above it:
# RailsWidget and InlineRailsWidget are for backward compatibility.
# New code should use Widget, InlineWidget, or Erector.inline.
I can (1) leave them in widget.rb, (2) put them in RailsWidget and we
can "un-deprecate" it, or (3) make them a new module that you can add
to your widget (Erector::RailsCompatibility or something?) if you need
them. I vote for (2) (as (3) just would be confusing -- two separate
"rails widget" classes/modules?), but I don't know the history behind
the deprecation of RailsWidget...
Andrew
John Firebaugh
Nov 23, 2009, 7:14:13 PM11/23/09
to Andrew Geweke, erector
>> The ignore_extra_controller_assigns getter and setter are
>> Rails-specific and should be in rails_widget.rb, no?
>
> Right now, they aren't, because RailsWidget has the following comment right
> above it:
>
> # RailsWidget and InlineRailsWidget are for backward compatibility.
> # New code should use Widget, InlineWidget, or Erector.inline.
RailsWidget, the class, is deprecated. rails_widget.rb, the file,
>> Rails-specific and should be in rails_widget.rb, no?
>
> Right now, they aren't, because RailsWidget has the following comment right
> above it:
>
> # RailsWidget and InlineRailsWidget are for backward compatibility.
> # New code should use Widget, InlineWidget, or Erector.inline.
isn't -- it contains the majority of the Rails-specific erector code.
(To reduce the confusion, the non-backward compatibility stuff should
probably be migrated out of there.)
The ignore_extra_controller_assigns stuff should specifically be in
the Erector::Rails::WidgetExtensions module, which is included in
Erector::Widget when erector is used from rails.
Andrew Geweke
Nov 23, 2009, 9:33:36 PM11/23/09
to John Firebaugh, erector
On Nov 23, 2009, at 4:14 PM, John Firebaugh wrote:
> RailsWidget, the class, is deprecated. rails_widget.rb, the file,
> isn't -- it contains the majority of the Rails-specific erector code.
> (To reduce the confusion, the non-backward compatibility stuff should
> probably be migrated out of there.)
>
> The ignore_extra_controller_assigns stuff should specifically be in
> the Erector::Rails::WidgetExtensions module, which is included in
> Erector::Widget when erector is used from rails.
OK, that was embarrassing...sorry about that. I managed to miss that
> RailsWidget, the class, is deprecated. rails_widget.rb, the file,
> isn't -- it contains the majority of the Rails-specific erector code.
> (To reduce the confusion, the non-backward compatibility stuff should
> probably be migrated out of there.)
>
> The ignore_extra_controller_assigns stuff should specifically be in
> the Erector::Rails::WidgetExtensions module, which is included in
> Erector::Widget when erector is used from rails.
completely the first time.
506663e7fdbbeb3c4706a4cfb7cb0839d7224ad5 (in my Erector fork) moves
the methods to Erector::Rails::WidgetExtensions, eliminates
NON_NEEDED_CONTROLLER_INSTANCE_VARIABLES, and makes
ignore_extra_controller_assigns default to true in Rails widgets.
I also committed 6294e5ad1bc468bb4d52050121264e02064767ab, which fixes
three (related) spec failures from the parent -- inline widgets didn't
work if you had the Rails code loaded. It'd be awesome if someone can
double-check that change (http://github.com/ageweke/erector/commit/6294e5ad1bc468bb4d52050121264e02064767ab
) -- I think it's pretty safe, but I always like getting feedback from
those who understand the code base better than I do.
What does everyone think? Looking better?
Andrew
Alex Chaffee
Dec 1, 2009, 4:17:32 PM12/1/09
to age...@gmail.com, John Firebaugh, erector
Andrew -
I just reviewed all your patches (sorry for the delay) and I'm OK with
them all. I didn't totally follow the ignore_extra_controller_assigns
discussion between you and John, but I'll assume you guys worked it
all out and do a bit of merging this afternoon.
- Alex
---
Alex Chaffee - al...@cohuman.com - http://alexch.github.com
Stalk me: http://friendfeed.com/alexch | http://twitter.com/alexch |
http://alexch.tumblr.com
> --
>
> You received this message because you are subscribed to the Google Groups "erector" group.
> To post to this group, send email to ere...@googlegroups.com.
> To unsubscribe from this group, send email to erector+u...@googlegroups.com.
> For more options, visit this group at http://groups.google.com/group/erector?hl=en.
>
>
>
I just reviewed all your patches (sorry for the delay) and I'm OK with
them all. I didn't totally follow the ignore_extra_controller_assigns
discussion between you and John, but I'll assume you guys worked it
all out and do a bit of merging this afternoon.
- Alex
---
Alex Chaffee - al...@cohuman.com - http://alexch.github.com
Stalk me: http://friendfeed.com/alexch | http://twitter.com/alexch |
http://alexch.tumblr.com
>
> You received this message because you are subscribed to the Google Groups "erector" group.
> To post to this group, send email to ere...@googlegroups.com.
> To unsubscribe from this group, send email to erector+u...@googlegroups.com.
> For more options, visit this group at http://groups.google.com/group/erector?hl=en.
>
>
>
John Firebaugh
Dec 1, 2009, 4:26:58 PM12/1/09
to Alex Chaffee, age...@gmail.com, erector
Looks good to me too. Thanks Andrew!
John
John
Alex Chaffee
Dec 1, 2009, 4:44:47 PM12/1/09
to John Firebaugh, age...@gmail.com, erector
I'm getting two failures in the Rails suite post-merge, both in render_spec:
1)
'ActionController::Base#render should raise when rendering a widget
class with implicit assigns and too many variables' FAILED
expected RuntimeError with message matching /Unknown parameter.*baz/,
got #<RuntimeError: Unknown parameter 'template'. NeedsWidget only
accepts foo, bar>
2)
'ActionController::Base#render should raise if rendering a #needs
template with excess controller variables' FAILED
expected ActionView::TemplateError with message matching /Unknown
parameter.*barfoo/, got #<ActionView::TemplateError:
ActionView::TemplateError (Unknown parameter 'template'.
Views::Test::Needs only accepts foobar, foobar) on line #1 of
app/views/test/needs.html.rb:
1: class Views::Test::Needs < Erector::Widget
2: needs :foobar
3:
4: def content
It seems that at one point you were omitting _template by force, and
then took that out in favor of the ignoring_extra_controller_assigns
setting, which these tests are now not setting. But I'm confused on
where to fix it -- should the tests be setting it themselves, or
should they be calling different controller methods, or should they
actually be expecting this new failure mode?
Andrew, could you see if you're getting these failures and if so give
me a hint on the right way to fix them?
1)
'ActionController::Base#render should raise when rendering a widget
class with implicit assigns and too many variables' FAILED
expected RuntimeError with message matching /Unknown parameter.*baz/,
got #<RuntimeError: Unknown parameter 'template'. NeedsWidget only
accepts foo, bar>
2)
'ActionController::Base#render should raise if rendering a #needs
template with excess controller variables' FAILED
expected ActionView::TemplateError with message matching /Unknown
parameter.*barfoo/, got #<ActionView::TemplateError:
ActionView::TemplateError (Unknown parameter 'template'.
Views::Test::Needs only accepts foobar, foobar) on line #1 of
app/views/test/needs.html.rb:
1: class Views::Test::Needs < Erector::Widget
2: needs :foobar
3:
4: def content
It seems that at one point you were omitting _template by force, and
then took that out in favor of the ignoring_extra_controller_assigns
setting, which these tests are now not setting. But I'm confused on
where to fix it -- should the tests be setting it themselves, or
should they be calling different controller methods, or should they
actually be expecting this new failure mode?
Andrew, could you see if you're getting these failures and if so give
me a hint on the right way to fix them?
Andrew Geweke
Dec 1, 2009, 5:05:02 PM12/1/09
to Alex Chaffee, John Firebaugh, erector
(6294e5ad1bc468bb4d52050121264e02064767ab). Are you merging my entire
tree, or just individual commits?
I'd make sure you're merging at least these commits:
dd7676fbe1da4260dc35cb9a76ee97c657aef058
5b5971eaa2302e65be01d6ed100f7167c38a6660
506663e7fdbbeb3c4706a4cfb7cb0839d7224ad5
6294e5ad1bc468bb4d52050121264e02064767ab
If you'll let me know which specific commits you're merging, I'd be
happy to give it a shot myself and fix any failures that are
happening. There's enough going on here that it can be a little
tricky, I think...
Thanks again!
Andrew
Alex Chaffee
Dec 1, 2009, 8:34:00 PM12/1/09
to erector
Turns out this commit of mine was causing the Rails render_spec to
fail: http://github.com/pivotal/erector/commit/ef4e4985c6e849f096c2435b025c7a925f9ea17f
Bizarre! This isn't changing anything outside the Form class...
I think I'll keep it reverted for now and do a release. There's some
good stuff in here, including a block-related bug fix of mine. 0.7.3,
here we come!
fail: http://github.com/pivotal/erector/commit/ef4e4985c6e849f096c2435b025c7a925f9ea17f
Bizarre! This isn't changing anything outside the Form class...
I think I'll keep it reverted for now and do a release. There's some
good stuff in here, including a block-related bug fix of mine. 0.7.3,
here we come!
Alex Chaffee
Dec 1, 2009, 9:11:20 PM12/1/09
to erector
Now CI is failing, even though it passes on my machine, and with the
suspicious commit reverted. WTF?
See http://ci.pivotallabs.com:3333/builds/Erector/5961e6a for details.
---
Alex Chaffee - al...@cohuman.com - http://alexch.github.com
Stalk me: http://friendfeed.com/alexch | http://twitter.com/alexch |
http://alexch.tumblr.com
suspicious commit reverted. WTF?
See http://ci.pivotallabs.com:3333/builds/Erector/5961e6a for details.
---
Alex Chaffee - al...@cohuman.com - http://alexch.github.com
Stalk me: http://friendfeed.com/alexch | http://twitter.com/alexch |
http://alexch.tumblr.com
Chad Woolley
Dec 1, 2009, 9:39:22 PM12/1/09
to ale...@gmail.com, erector
On Tue, Dec 1, 2009 at 7:11 PM, Alex Chaffee <ale...@gmail.com> wrote:
> Now CI is failing, even though it passes on my machine, and with the
> suspicious commit reverted. WTF?
>
> See http://ci.pivotallabs.com:3333/builds/Erector/5961e6a for details.
Is your build environment identical? Verify by putting some env
> Now CI is failing, even though it passes on my machine, and with the
> suspicious commit reverted. WTF?
>
> See http://ci.pivotallabs.com:3333/builds/Erector/5961e6a for details.
checks in your build script, like this:
http://github.com/rails/rails/blob/28657e4f418c84dad08ae1f541d24df2d91d93aa/ci/ci_build.rb#L100
-- Chad
Alex Chaffee
Dec 2, 2009, 1:05:57 AM12/2/09
to Chad Woolley, erector
Thanks for the tip. That was fun but didn't really help much, since
the thing happened and also didn't happen on my laptop at different
times. The CI box is on 1.8.6 btw -- shouldn't it be on 1.8.7 by now?
---
Alex Chaffee - al...@cohuman.com - http://alexch.github.com
Stalk me: http://friendfeed.com/alexch | http://twitter.com/alexch |
http://alexch.tumblr.com
the thing happened and also didn't happen on my laptop at different
times. The CI box is on 1.8.6 btw -- shouldn't it be on 1.8.7 by now?
---
Alex Chaffee - al...@cohuman.com - http://alexch.github.com
Stalk me: http://friendfeed.com/alexch | http://twitter.com/alexch |
http://alexch.tumblr.com
Chad Woolley
Dec 2, 2009, 10:00:19 AM12/2/09
to Alex Chaffee, erector
On Tue, Dec 1, 2009 at 11:05 PM, Alex Chaffee <ale...@gmail.com> wrote:
> The CI box is on 1.8.6 btw -- shouldn't it be on 1.8.7 by now?
Probably. I'd have to do some research on what would break, or at
> The CI box is on 1.8.6 btw -- shouldn't it be on 1.8.7 by now?
least maybe bug people to get things green on 1.8.6 first. It's still
a shared box.
BTW, getting on Bundler is a great thing to do if you haven't yet...
Alex Chaffee
Dec 3, 2009, 2:21:55 AM12/3/09
to Andrew Geweke, erector
I just upgraded to rails 2.3.5... and now the 2 tests are failing for
me locally again. That's good news, right? Andrew, can you try to
'sudo gem update rails' and see what happens?
---
Alex Chaffee - al...@cohuman.com - http://alexch.github.com
Stalk me: http://friendfeed.com/alexch | http://twitter.com/alexch |
http://alexch.tumblr.com
me locally again. That's good news, right? Andrew, can you try to
'sudo gem update rails' and see what happens?
---
Alex Chaffee - al...@cohuman.com - http://alexch.github.com
Stalk me: http://friendfeed.com/alexch | http://twitter.com/alexch |
http://alexch.tumblr.com
Alex Chaffee
Dec 3, 2009, 10:30:39 AM12/3/09
to Andrew Geweke, erector
Okay, I worked it out.
ActionController::Base defines the following
(action_pack/lib/action_controller/base.rb line 256)
@@protected_instance_variables = %w(@assigns @performed_redirect
@performed_render @variables_added @request_origin @url
@parent_controller
@action_name
@before_filter_chain_aborted @action_cache_path @_session @_headers
@_params
@_flash @_response)
but this list does not include @real_format or @_request, so they trip
up the 'needs' alert
so I removed them myself from inside Erector's monkey patchy
ActionView::Base#instance_variables_for_widget_assignment_for
Seems like a Rails bug. Anyone feel like submitting a patch?
- A
P.S. Make sure to 'sudo gem update rails' or just run 'sudo
geminstaller' to get Rails 2.3.5 all installed on your system before
doing more erector+rails development.
---
Alex Chaffee - al...@cohuman.com - http://alexch.github.com
Stalk me: http://friendfeed.com/alexch | http://twitter.com/alexch |
http://alexch.tumblr.com
ActionController::Base defines the following
(action_pack/lib/action_controller/base.rb line 256)
@@protected_instance_variables = %w(@assigns @performed_redirect
@performed_render @variables_added @request_origin @url
@parent_controller
@action_name
@before_filter_chain_aborted @action_cache_path @_session @_headers
@_params
@_flash @_response)
but this list does not include @real_format or @_request, so they trip
up the 'needs' alert
so I removed them myself from inside Erector's monkey patchy
ActionView::Base#instance_variables_for_widget_assignment_for
Seems like a Rails bug. Anyone feel like submitting a patch?
- A
P.S. Make sure to 'sudo gem update rails' or just run 'sudo
geminstaller' to get Rails 2.3.5 all installed on your system before
doing more erector+rails development.
---
Alex Chaffee - al...@cohuman.com - http://alexch.github.com
Stalk me: http://friendfeed.com/alexch | http://twitter.com/alexch |
http://alexch.tumblr.com
Andrew Geweke
Dec 3, 2009, 5:18:38 PM12/3/09
to Alex Chaffee, erector
Sorry it took me a bit to respond -- my day yesterday was totally
shot, and trying to avoid flooding the list with too much email. Feel
free to IM me (age...@gmail.com) if anybody wants a quicker ping at
any point.
So, the mystery continues re: specs. On my laptop, using the pivotal/
erector tree, I get two failures (attached as 'spec-failures.txt') at
the head, e352ca9d3a1ce789. I've checked in my patch for these at http://github.com/ageweke/erector/commit/a1c142738b89dbde1ad949c128e582cc05282318
; it seems like more missing Rails variables, although I'm at a loss
to explain why I get them and the CI box doesn't. Maybe it's 1.8.6 vs.
1.8.7? I'm on Snow Leopard and thus fixed to 1.8.7 (I'm unaware of a
1.8.6 Ruby that compiles and runs correctly on SL). The patch seems
like it should be safe everywhere, though, so maybe we still want it?
Another issue: 'rake install_dependencies' doesn't work for me, and
never has. I've checked in a patch for this at http://github.com/ageweke/erector/commit/4a2a254be8db6602f0f0d64972f2208e4fecba16
; the comment I added explains what's going on and why. (It's an
unfortunate interaction with the 'git' gem that Jeweler uses.) I'm
really curious why others aren't seeing this -- I've got gems: git =>
1.2.5, technicalpickles-jeweler => 1.2.1.
I will dig into the Rails bug you mention (@real_format, @_request,
and now @template and @request, as well), see what's going on, and
send in a patch if appropriate. We're trying to put a tighter
interface between Rails and the views than ERb uses, so they have no
tests that would cover this.
I've also merged the head of pivotal/erector into ageweke/erector, so
I believe we should be in sync completely. Onwards!
Andrew
shot, and trying to avoid flooding the list with too much email. Feel
free to IM me (age...@gmail.com) if anybody wants a quicker ping at
any point.
So, the mystery continues re: specs. On my laptop, using the pivotal/
erector tree, I get two failures (attached as 'spec-failures.txt') at
the head, e352ca9d3a1ce789. I've checked in my patch for these at http://github.com/ageweke/erector/commit/a1c142738b89dbde1ad949c128e582cc05282318
; it seems like more missing Rails variables, although I'm at a loss
to explain why I get them and the CI box doesn't. Maybe it's 1.8.6 vs.
1.8.7? I'm on Snow Leopard and thus fixed to 1.8.7 (I'm unaware of a
1.8.6 Ruby that compiles and runs correctly on SL). The patch seems
like it should be safe everywhere, though, so maybe we still want it?
Another issue: 'rake install_dependencies' doesn't work for me, and
never has. I've checked in a patch for this at http://github.com/ageweke/erector/commit/4a2a254be8db6602f0f0d64972f2208e4fecba16
; the comment I added explains what's going on and why. (It's an
unfortunate interaction with the 'git' gem that Jeweler uses.) I'm
really curious why others aren't seeing this -- I've got gems: git =>
1.2.5, technicalpickles-jeweler => 1.2.1.
I will dig into the Rails bug you mention (@real_format, @_request,
and now @template and @request, as well), see what's going on, and
send in a patch if appropriate. We're trying to put a tighter
interface between Rails and the views than ERb uses, so they have no
tests that would cover this.
I've also merged the head of pivotal/erector into ageweke/erector, so
I believe we should be in sync completely. Onwards!
Andrew
Alex Chaffee
Dec 3, 2009, 6:10:17 PM12/3/09
to Andrew Geweke, erector
Sorry, I just broke the build... please ignore 85de079 until I get a
chance to fix it...
---
Alex Chaffee - al...@cohuman.com - http://alexch.github.com
Stalk me: http://friendfeed.com/alexch | http://twitter.com/alexch |
http://alexch.tumblr.com
chance to fix it...
---
Alex Chaffee - al...@cohuman.com - http://alexch.github.com
Stalk me: http://friendfeed.com/alexch | http://twitter.com/alexch |
http://alexch.tumblr.com
Reply all
Reply to author
Forward
0 new messages