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

integrated with plush's own partial (and added testcases) #1411

Merged
merged 1 commit into from Nov 1, 2018

Conversation

Projects
None yet
2 participants
@sio4
Copy link
Contributor

commented Oct 26, 2018

Hi, @markbates and all,

Added a test case for issue #1406. I think the solution to this issue should be in plush and I tested with patch below:

--- a/compiler.go
+++ b/compiler.go
@@ -504,6 +504,21 @@ func (c *compiler) evalCallExpression(node *ast.CallExpression) (interface{}, er
                                return nil, errors.WithStack(err)
                        }
 
+                       // 3. if the function is `partial` and user provides second argument,
+                       //    merge it with current context's data which has loop var.
+                       if node.Function.String() == "partial" && pos == 1 {
+                               fmt.Printf("XXX - %T, %v\n", v, v)
+                               data := map[string]interface{}{}
+                               for k, vv := range c.ctx.data {
+                                       data[k] = vv
+                               }
+                               for k, vv := range v.(map[string]interface{}) {
+                                       data[k] = vv
+                               }
+                               v = data
+                               fmt.Printf("XXX - %T, %v\n", v, v)
+                       }
+
                        var ar reflect.Value
                        expectedT := rt.In(pos)
                        if v != nil {
@@ -520,6 +535,12 @@ func (c *compiler) evalCallExpression(node *ast.CallExpression) (interface{}, er
                        args = append(args, ar)
                }
 
+               // 3.1. if the function is `partial` but there is no second argument,
+               //      just use current context's data as second argument.
+               if node.Function.String() == "partial" && len(node.Arguments) == 1 {
+                       args = append(args, reflect.ValueOf(c.ctx.data))
+               }
+
                hc := func(arg reflect.Type) {
                        if arg.ConvertibleTo(reflect.TypeOf(HelperContext{})) {
                                hargs := HelperContext{
@@ -619,6 +640,7 @@ func (c *compiler) evalCallExpression(node *ast.CallExpression) (interface{}, er
                }
        }
 
+       // 4. finally it calls `partial()` but it runs on another context.
        res := rv.Call(args)
        if len(res) > 0 {
                if e, ok := res[len(res)-1].Interface().(error); ok {
@@ -634,6 +656,8 @@ func (c *compiler) evalForExpression(node *ast.ForExpression) (interface{}, erro
        defer func() {
                c.ctx = octx
        }()
+       // 1. create new context for `for` block here.
+       //    anyway this context does not have values form original.
        c.ctx = octx.New()
        iter, err := c.evalExpression(node.Iterable)
        if err != nil {
@@ -661,8 +685,9 @@ func (c *compiler) evalForExpression(node *ast.ForExpression) (interface{}, erro
        case reflect.Slice, reflect.Array:
                for i := 0; i < riter.Len(); i++ {
                        v := riter.Index(i)
-                       c.ctx.Set(node.KeyName, i)
+                       c.ctx.Set(node.KeyName, i) // misc: is it required?
                        c.ctx.Set(node.ValueName, v.Interface())
+                       // 2. it calls evalBlockStatement after set new values
                        res, err := c.evalBlockStatement(node.Block)
                        if err != nil {
                                return nil, errors.WithStack(err)

@sio4 sio4 requested a review from gobuffalo/core-managers as a code owner Oct 26, 2018

@sio4

This comment has been minimized.

Copy link
Contributor Author

commented Oct 26, 2018

Travis and AppVeyor failed on these test cases. To solve issue #1406, it should be passed.

@markbates

This comment has been minimized.

Copy link
Member

commented Oct 26, 2018

@sio4 this is great!

Thinking about what you've dug up, I agree that Plush should have a partial helper. It should have the signature as the one in Buffalo, but the different is the string it takes isn't a file name, but the HTML to be rendered. Then in Buffalo we can wrap that in our own partial function that gets the file, and sends it all onto the plush one.

Does that sound right to you? If so, I would love it if you could own this. :)

@sio4

This comment has been minimized.

Copy link
Contributor Author

commented Oct 26, 2018

Hi @markbates
I am sorry but I cannot fully understand your comment. (since my English is very poor)

Anyway, This PR is for testing the partial is working or not within for loop. If you tried this tests, or just simply checking the Travis-CI's output with the test case codes in this commit, you can find the issue still exists although you did patch. (The point of this PR and my comments is not about a position of partial() but the problem.)

For the implementation of partial, as I already wrote as comments on the issue, #1406 (comment) ,
I understood why partial() was implemented on Buffalo side since it needs to handle file i/o and understands directory hierarchy of the application, and plush just handle given string without files and directory paths. So I fully agree with the current "call-back style" structure.

But since it still not working, we need to fix it and I think the patch for plush on above comment is one of the possible approaches.

If you agree, I will open another PR for plush.

@markbates

This comment has been minimized.

Copy link
Member

commented Oct 26, 2018

No problem. :) I was agreeing 100% with you. 👍

  • fix plush
  • add partial(htmlBody string, options, ctx) helper to plush
  • change buffalo's partial to call plush's partial function with the template file read from disk as a string

does that help?

@sio4

This comment has been minimized.

Copy link
Contributor Author

commented Oct 29, 2018

Yes, I think that will help.
Additionally, to keep compatibility and to keep it simple, I think the context should be held by the compiler as current, and pass the function pointer to application's partial feeder so it can return to compiler's context.

Anyway, currently, that feature is broken status so we need to fix it first.

@sio4

This comment has been minimized.

Copy link
Contributor Author

commented Oct 29, 2018

@markbates Something like this?

In plush side, add this and register it as "partial" helper:

func partialHelper(name string, data map[string]interface{}, help HelperContext) (template.HTML, error) {
	for k, v := range data {
		help.Context.data[k] = v
	}

	f := reflect.ValueOf(help.Context.data["partialFeeder"])
	args := []reflect.Value{reflect.ValueOf(name)}
	res := f.Call(args)

	body, err := Render(res[0].Interface().(string), help.Context)

	if data["layout"] != nil {
		return partialHelper(
			data["layout"].(string),
			map[string]interface{}{"yield": template.HTML(body)},
			help,
		)
	}

	return template.HTML(body), err
}

(before call it from for loop, all context data should be copied from parent.)

and in application side, implement something like this and register as "partialFeeder" helper:

func (s templateRenderer) partialFeeder(name string) (string, error) {
       d, f := filepath.Split(name)
       name = filepath.Join(d, "_"+f)

       if filepath.Ext(name) == "" {
               name += ".html"
       }

       source, err := s.TemplatesBox.MustBytes(name)
       if err != nil {
               return "", err
       }

       return string(source), nil
}

I didn't see in details but it passed most test cases of buffalo's render/ but some javascript related test cases are failed. (I removed partial helper from buffalo side for testing above)

@sio4

This comment has been minimized.

Copy link
Contributor Author

commented Oct 29, 2018

Some fix for JS:

	ct := help.Context.data["contentType"]
	if ct != nil && strings.Contains(ct.(string), "javascript") && strings.Contains(name, "html") {
		body = template.JSEscapeString(string(body))
	}

The codes above just before returning body from partialHelper() helps JS things.

This approach breaks current calling structure but I think we cannot avoid changes on both plush and buffalo if we move partial to plush side.
I think this approach is better than plush's compiler calls application's partial implementation and it calls again plush's new partial. With this, application developer just thinks about feeding.

@sio4

This comment has been minimized.

Copy link
Contributor Author

commented Oct 30, 2018

Hi @markbates, how do you think about my approach? If you agree, I will start to code it.

Please remember, the partial within for is not working currently! It needs hotfix before we get the direction for the future!

@markbates

This comment has been minimized.

Copy link
Member

commented Oct 30, 2018

Hi @sio4 sorry I haven't responded earlier, but I've been busy, and I don't think this is as wide spread and urgent as you think, as no one else is complaining off it, however, I agree it needs fixing, and I would like to get it into a v0.13.x release.

A few questions:

  • is this a breaking change? I don't think it is, but I want to confirm
  • can we change the partialFeeder stuff to not use reflection? you can use something like pf, ok := data['feeder'].(plush.Feeder) not proper code, but hopefully you understand.

If this isn't a breaking change, then please, start ASAP. :)

@sio4

This comment has been minimized.

Copy link
Contributor Author

commented Oct 31, 2018

Hi @markbates,
For your first questions, I also think that it should not affect to existing user's applications. it is not breaking change in point of user's applications. but since it is related to both Buffalo and Plush, and I think calling flow should be changed as the idea I shared, it needs to versioning as you always did and changed seamlessly on Buffalo side and Plush side.
For the second one, OK, I will try it to make it simpler as you said.

Let's start!

@sio4 sio4 referenced this pull request Oct 31, 2018

Merged

added partial helper #72

@sio4 sio4 changed the title added testcase for partial within for loop integrated with plush's own partial (and added testcases) Oct 31, 2018

@sio4

This comment has been minimized.

Copy link
Contributor Author

commented Oct 31, 2018

I pushed two PRs on buffalo and plush. PR on plush is just adding a new helper and this PR uses that change. (so currently related test cases are failed on CIs, but I tested it locally.)

As @markbates asked, these changes do not affect other applications using plush but buffalo should be updated with this PR. I also removed codes using reflection and changed with as your recommendation.

Please check this.

@markbates markbates changed the base branch from development to master Oct 31, 2018

@markbates markbates changed the base branch from master to development Oct 31, 2018

@markbates

This comment has been minimized.

Copy link
Member

commented Oct 31, 2018

@sio4 can you rebase this against master? I want to cut a v0.13.x release with this, but development is not ready to be released. Thanks.

@markbates markbates added this to the v0.13.3 milestone Oct 31, 2018

@sio4

This comment has been minimized.

Copy link
Contributor Author

commented Oct 31, 2018

@markbates

This comment has been minimized.

Copy link
Member

commented Oct 31, 2018

@sio4 sio4 force-pushed the gochigo:partial_with_for_loop branch from d5e1dd0 to f23f730 Oct 31, 2018

@sio4 sio4 changed the base branch from development to master Oct 31, 2018

@sio4 sio4 changed the base branch from master to development Oct 31, 2018

@sio4 sio4 changed the base branch from development to master Oct 31, 2018

@sio4

This comment has been minimized.

Copy link
Contributor Author

commented Oct 31, 2018

Hi @markbates, I pushed new commits with the same changes against master, and just after the push, I changed the base branch. Maybe that reason, CI builds are failed. Can you trigger them manually?

@markbates markbates closed this Oct 31, 2018

@markbates markbates reopened this Oct 31, 2018

@markbates

This comment has been minimized.

Copy link
Member

commented Oct 31, 2018

I briefly closed/re-opened the PR to see if that will trigger CI

@markbates markbates merged commit e9e3594 into gobuffalo:master Nov 1, 2018

3 checks passed

codeclimate 2 fixed issues
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@sio4

This comment has been minimized.

Copy link
Contributor Author

commented Nov 1, 2018

Thanks!

@lukasschlueter lukasschlueter referenced this pull request Nov 20, 2018

Closed

markdown style #404

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.