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

Failing test for yielding label block #76

Merged

Conversation

seanpdoyle
Copy link
Collaborator

When yielding a block named label (via partial.label.yield), an argument error is raised.

Error:
RendererTest#test_render_partial_can_yield_block_named_label:
ArgumentError: wrong number of arguments (given 1, expected 2..4)
    test/renderer_test.rb:20:in `block (2 levels) in <class:RendererTest>'
    test/renderer_test.rb:19:in `block in <class:RendererTest>'

When yielding a block named `label` (via `partial.label.yield`), an
argument error is raised.

```
Error:
RendererTest#test_render_partial_can_yield_block_named_label:
ArgumentError: wrong number of arguments (given 1, expected 2..4)
    test/renderer_test.rb:20:in `block (2 levels) in <class:RendererTest>'
    test/renderer_test.rb:19:in `block in <class:RendererTest>'
```
@seanpdoyle
Copy link
Collaborator Author

@kaspth I'm not sure what's at the root of this, but I encountered it by chance.

@seanpdoyle seanpdoyle marked this pull request as ready for review January 18, 2023 20:19
@seanpdoyle
Copy link
Collaborator Author

@kaspth the following diff passes the tests:

--- a/lib/nice_partials/partial.rb
+++ b/lib/nice_partials/partial.rb
@@ -95,11 +95,7 @@ module NicePartials
     end
 
     def method_missing(meth, *arguments, **keywords, &block)
-      if @view_context.respond_to?(meth)
-        @view_context.public_send(meth, *arguments, **keywords, &block)
-      else
-        define_accessor meth and public_send(meth, *arguments, **keywords, &block)
-      end
+      define_accessor meth and public_send(meth, *arguments, **keywords, &block)
     end
 
     def define_accessor(name)

It turns out that we were optimistically delegating to ActionView::Base whenever we could, instead of defining named sections.

Is this change too aggressive? Is there a gap in our test coverage of this use case, or are we safe to remove the delegation?

Prior to this commit, the `Partial` instance optimistically delegated to
the `ActionView::Base` instance referenced by the `@view_context`
variable, instead of defining named `Section` instances.

This commit removes that delegation.
@kaspth
Copy link
Contributor

kaspth commented Jan 31, 2023

Is this change too aggressive? Is there a gap in our test coverage of this use case, or are we safe to remove the delegation?

It's forpartial.helpers { def something = root_url }, so we can call view context methods without needing to be aware of it. I think we should add a test case for that behavior instead of removing the delegation.

I have an alternate idea where we can maybe remove the delegation need. Let me try that and submit a PR. I'll cc you on it.

@seanpdoyle seanpdoyle changed the base branch from main to restructure-helpers-block January 31, 2023 21:53
@seanpdoyle
Copy link
Collaborator Author

@kaspth I've changed this PR's target branch.

@kaspth kaspth merged commit d5f8279 into bullet-train-co:restructure-helpers-block Jan 31, 2023
@seanpdoyle seanpdoyle deleted the label-bug branch January 31, 2023 21:56
kaspth added a commit that referenced this pull request Feb 2, 2023
* Restructure helpers to use a cleanroom object

* Failing test for yielding `label` block (#76)

---------

Co-authored-by: Sean Doyle <seanpdoyle@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants