Implement Service() #1139

tchssk · 2017-03-23T14:34:14Z

raphael

Looks good! Left a few small comments.
One thing that might be missing is the case where a payload or result type uses another type in the design. In this case that other type also needs to be generated. Seems like the code needs to traverse all the attributes recursively and collect all the types (using Walk https://github.com/goadesign/goa/blob/v2/design/types.go#L68).

codegen/files/service.go

+	{
+		header = codegen.Header(s.service.Name+"Services", "services",
+			[]*codegen.ImportSpec{
+				&codegen.ImportSpec{Path: "context"},


codegen/files/service.go

+		methods := make([]*serviceMethod, len(s.service.Endpoints))
+		for i, v := range s.service.Endpoints {
+			fields := make(map[string]string)
+			if o, ok := v.Payload.Attribute().Type.(design.Object); ok {


codegen/files/service.go

+const serviceT = `type (
+	// {{ .VarName }} is the {{ .Name }} service interface.
+	{{ .VarName }} interface {
+{{ range .Methods }}		// {{ .Name }} implements the {{ .Name }} endpoint.


codegen/files/service_test.go

+				&b,
+			},
+		}
+		_ = multipleEndpoints


raphael · 2017-03-24T17:52:24Z

Thanks for all the changes, looks good. Are you working on recursively tracking the types that need to be generated? seems like it's the last item remaining.

tchssk · 2017-03-26T01:23:31Z

Yes, I'm working on that.

It seems we have to improve GoTypeName() and GoNativeType() before using Walk() so I'll add some commits first.

tchssk · 2017-03-26T02:05:29Z

I'm going forward step by step so I pushed commits about GoNativeType first. Please take a look these commits.

de8d8c6 adds tests for GoNativeType().
abddedc supports Map, Object and UserType in GoNativeType().

raphael · 2017-03-27T05:59:14Z

The GoNativeType implementation and tests look good. Something I've been wondering is whether we should add back DateTime as a type in v2. I was thinking initially that it could just be String with a special datetime format. But after thinking it through and looking at real APIs it seems having a DateTime as first class type in the design might be better. What do you think?

tchssk · 2017-03-27T06:32:18Z

v1 has DateTimeKind and has DateTime as Primitive so we can design v2 as same way as that. Maybe it's useful for OpenAPI (Swagger) but v2 supports also gRPC.

It's a big topic for v2. Do we open a new issue and discuss about that? At this point, I can continue this PR to support only existing types and we can add more types support as needed in the future.

tchssk · 2017-03-27T12:16:11Z

I pushed a new commit.

3264349 tracks the types recursively in Service().

tchssk · 2017-03-28T03:39:10Z

In the way you shown, I could track only top level fields (means Parent in your example) in the payload. It seems we have to track the types recursively using ut.Walk().

We also might need a function like GoTypeDef() in v1 to write each type definitions.

raphael · 2017-03-28T17:43:17Z

Yes you're right - it has to be recursive. And also agree on needing something like GoTypeDef. Last commit looks great BTW.

tchssk · 2017-03-30T03:13:20Z

I pushed new commits.

41a41a2 implements GoTypeDef().
9c6ddd8 renders results in Service().
47757ca tracks the types recursively in Service().

I didn't add an argument to indent codes in GoTypeDef() but the codes will be gofmt ed, right?

raphael · 2017-03-30T15:20:15Z

Looks good overall! a couple of comments:

Cycles

I think the walker code needs to handle cycles, consider this design:

var Parent = Type("parent", func() {
    Attribute("c", Child)
})

var Child = Type("child", func() {
    Attribute("p", "parent")
})

This is a legal definition that will cause the walker to recurse infinitely. I think it needs to recurse only if the type is not already in the hash.

Indenting

Yes the code gets gofmted however the idea so far has been to render code that was properly formatted and use gofmt as a last resort. However it might be better to give up on trying to generate code that is properly indented and just rely on gofmt. The added complexity involved in generating the proper level of indentation (esp. with recursive algorithms) may not be worth it. However if we do that we should do it consistently everywhere.

So @michaelboke @tchssk let's do a quick vote here :) do we indent the generated code using gofmt or via the templates? I vote gofmt.

tchssk · 2017-03-30T16:05:58Z

gofmt +1

tchssk · 2017-03-30T16:09:44Z

Cycles

ea09cbe avoids infinite recursion in Service().

michaelboke · 2017-03-30T16:13:34Z

Gofmt but keep formatting as good as possible in templates. At least I'm trying Op do 30 mrt. 2017 om 18:09 schreef Taichi Sasaki <notifications@github.com>

…

Cycles ea09cbe <ea09cbe> avoids infinite recursion in Service(). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1139 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACBg1F4dyf-OTqPCpqIU9OVenrlVoWmFks5rq9PJgaJpZM4Mm1jw> .

raphael · 2017-03-30T16:16:17Z

Agree on keeping formatting as good as possible. The idea here is mainly to remove the tracking of indentation - that is remove the depth argument from all the template functions and instead always format properly but with a level of indentation of 0.

tchssk · 2017-03-30T16:25:52Z

I tried to write the codes from the idea.

f0ed658 removes indents and use gofmt for tests.

or another implementation.

f628868 formats codes by gofmt in Section.Write().

tchssk · 2017-04-07T01:15:40Z

Once it seems finished. or Do I have anything to do more?

raphael

Looks great! thank you. It might be good to have a few more tests that cover all the case, something like:

With empty payload, empty result
With non empty payload but empty result
With empty payload and non empty result
With non empty payload and non empty result.

For the non empty tests it would also be good to cover all the data types (primitive, array, map, object, user and media types).

The tests here are good, I'm hoping we can be systematic and use variable names that are consistent for the others as well (like you have with APayload etc.).

codegen/types.go

+		return fmt.Sprintf("map[%s]%s", keyDef, elemDef)
+	case design.Object:
+		return goTypeDefObject(actual)
+	case *design.UserTypeExpr:


codegen/types.go

+		return fmt.Sprintf("map[%s]%s", GoNativeType(actual.KeyType.Type), GoNativeType(actual.ElemType.Type))
+	case design.Object:
+		return "map[string]interface{}"
+	case *design.UserTypeExpr:


codegen/types.go

+		return fmt.Sprintf("map[%s]%s", GoNativeType(actual.KeyType.Type), GoNativeType(actual.ElemType.Type))
+	case design.Object:
+		return "map[string]interface{}"
+	case *design.UserTypeExpr:


codegen/types.go

@@ -143,7 +143,7 @@ func goTypeDefObject(o design.Object) string {
 	var ss []string
 	ss = append(ss, "struct {")
 	o.WalkAttributes(func(name string, at *design.AttributeExpr) error {
-		ss = append(ss, fmt.Sprintf("%s %s", name, GoTypeName(at.Type)))
+		ss = append(ss, fmt.Sprintf("\t%s %s", name, GoTypeName(at.Type)))


tchssk · 2017-04-09T13:21:36Z

a97bc6b uses UserType instead of UserTypeExpr.

codegen/types.go

-	case *design.UserTypeExpr:
-		return GoTypeName(actual)
+	case design.UserType:
+		return GoTypeName(actual.Attribute().Type)


tchssk · 2017-04-17T16:34:58Z

fc7a5b1 adds tests about empty payload and empty result.

tchssk · 2017-04-18T08:09:01Z

Travis failed with error that WalkAttributes() methods of design.Object not found. This seems to be caused by #1164.

raphael · 2017-04-18T14:27:48Z

Yes this PR moved the walkers to the code generation package (files.WalkAttributes) because the design and DSL engine don't use it, only code generation does.

tchssk · 2017-04-18T14:46:58Z

Shoud I rebase this branch onto v2?

raphael · 2017-04-18T16:22:22Z

Yeah, that would probably make sense

raphael · 2017-04-18T20:19:37Z

Actually I'll merge and rebase since I need some of the changes in this PR (the GoTypeName changes).

* Implement Service() * Use design.AsObject() instead of type assertion * Elide the respective literal type * Delete debug codes * Use Name and VarName differently in serviceMethod * Add tests for GoNativeType() * Support Map, Object and UserType in GoNativeType() * Track the types recursively in Service() * Separate template using define * Implement GoTypeDef() * Render results in Service() * Track the types recursively in Service() * Avoid infinite recursion in Service() * Remove indents and use gofmt for tests * Set indents in goTypeDefObject() * Set indents in Service() * Use UserType instead of UserTypeExpr * Fix GoTypeDef() about UserType * Add tests about empty payload and empty result

tchssk added the v2 label Mar 23, 2017

tchssk requested a review from raphael March 23, 2017 14:34

tchssk force-pushed the v2-writers-service branch from 806bcf5 to 13ac7d5 Compare March 23, 2017 14:45

Implement Service()

c6a1636

tchssk force-pushed the v2-writers-service branch from 13ac7d5 to c6a1636 Compare March 23, 2017 14:48

raphael reviewed Mar 23, 2017

View reviewed changes

tchssk added 4 commits March 24, 2017 10:07

Use design.AsObject() instead of type assertion

c9581fe

Elide the respective literal type

76a17d3

Delete debug codes

2c93fdb

Use Name and VarName differently in serviceMethod

5d6a618

raphael assigned tchssk Mar 25, 2017

Add tests for GoNativeType()

de8d8c6

tchssk force-pushed the v2-writers-service branch from 6587e5c to ce94d24 Compare March 26, 2017 05:16

Support Map, Object and UserType in GoNativeType()

abddedc

tchssk force-pushed the v2-writers-service branch from ce94d24 to abddedc Compare March 26, 2017 05:17

Track the types recursively in Service()

3264349

Separate template using define

c10f6af

tchssk added 3 commits March 30, 2017 11:57

Implement GoTypeDef()

41a41a2

Render results in Service()

9c6ddd8

Track the types recursively in Service()

47757ca

Avoid infinite recursion in Service()

ea09cbe

tchssk force-pushed the v2-writers-service branch from 07a5805 to ea09cbe Compare March 30, 2017 16:08

Remove indents and use gofmt for tests

f0ed658

tchssk added 2 commits April 6, 2017 10:17

Set indents in goTypeDefObject()

391dd0d

Set indents in Service()

867a755

raphael reviewed Apr 7, 2017

View reviewed changes

michaelboke suggested changes Apr 7, 2017

View reviewed changes

Use UserType instead of UserTypeExpr

a97bc6b

raphael reviewed Apr 10, 2017

View reviewed changes

tchssk added 2 commits April 17, 2017 19:03

Fix GoTypeDef() about UserType

caab119

Add tests about empty payload and empty result

fc7a5b1

raphael merged commit 444cdf1 into goadesign:v2 Apr 18, 2017

tchssk deleted the v2-writers-service branch April 25, 2017 13:27

Implement Service() #1139

Implement Service() #1139

Conversation

tchssk commented Mar 23, 2017

raphael left a comment

Choose a reason for hiding this comment

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

raphael commented Mar 24, 2017

tchssk commented Mar 26, 2017 • edited

tchssk commented Mar 26, 2017 • edited

raphael commented Mar 27, 2017

tchssk commented Mar 27, 2017

tchssk commented Mar 27, 2017

tchssk commented Mar 28, 2017

raphael commented Mar 28, 2017

tchssk commented Mar 30, 2017

raphael commented Mar 30, 2017 • edited

tchssk commented Mar 30, 2017

tchssk commented Mar 30, 2017

michaelboke commented Mar 30, 2017 via email

raphael commented Mar 30, 2017

tchssk commented Mar 30, 2017 • edited

tchssk commented Apr 7, 2017

raphael left a comment

Choose a reason for hiding this comment

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

tchssk commented Apr 9, 2017

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

tchssk commented Apr 17, 2017

tchssk commented Apr 18, 2017 • edited

raphael commented Apr 18, 2017

tchssk commented Apr 18, 2017

raphael commented Apr 18, 2017

raphael commented Apr 18, 2017

tchssk commented Mar 26, 2017 •

edited

tchssk commented Mar 26, 2017 •

edited

raphael commented Mar 30, 2017 •

edited

tchssk commented Mar 30, 2017 •

edited

tchssk commented Apr 18, 2017 •

edited