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

Add an argument to CollectionOf() #1232

Merged
merged 2 commits into from May 20, 2017

Conversation

Projects
None yet
2 participants
@tchssk
Copy link
Member

tchssk commented May 19, 2017

It makes it possible to specify the collection identifier.

Add an argument to CollectionOf()
It makes it possible to specify the collection identifier.

@tchssk tchssk requested a review from raphael May 19, 2017

@tchssk

This comment has been minimized.

Copy link
Member

tchssk commented May 19, 2017

I don't know why travis was failed :(

@raphael
Copy link
Member

raphael left a comment

The changes look great! I just suggested some tweaks to the comments, thank you.

for _, p := range paramAndDSL {
param, ok = p.(string)
if !ok {
dslengine.ReportError("invalid CollectionOf argument, must be a string", p)

This comment has been minimized.

@raphael

raphael May 19, 2017

Member

Seems like the error message should be must be a string or a DSL function.

})
})

Context("defined with the collectio identifier", func() {

This comment has been minimized.

@raphael

raphael May 19, 2017

Member

typo collectio -> collection. Also the -> a.

func CollectionOf(v interface{}, apidsl ...func()) *design.MediaTypeDefinition {
//
// CollectionOf(BottleMedia) // If the identifier of BottleMedia is "vnd.goa.bottle',
// // Content-Type will be "vnd.goa.bottle; type=collection".

This comment has been minimized.

@raphael

raphael May 19, 2017

Member

Should be identifier not Content-Type I think. By default the identifier is used to build the content type but that can be overridden with ContentType. So maybe something like:

// Examples:
//
//   // Define a collection media type using the default generated identifier
//   // (e.g. "vnd.goa.bottle; type=collection" assuming the identifier of BottleMedia
//   // is "vnd.goa.bottle") and the default views (i.e. inherited from the BottleMedia
//   // views).
//   var col = CollectionOf(BottleMedia)
//
//   // Another collection media type using the same element media type but defining a
//   // a different default view.
//   var col2 = CollectionOf(BottleMedia, "vnd.goa.bottle.alternate; type=collection;", func() {
//        View("default", func() {
//           Attribute("id")
//           Attribute("name")
//        })
//   })
@@ -376,9 +376,17 @@ func Link(name string, view ...string) {
// CollectionOf creates a collection media type from its element media type. A collection media

This comment has been minimized.

@raphael

raphael May 19, 2017

Member

Please remove the first 3 lines:

// CollectionOf can be used in: Wherever a MediaType can be used..
 //
 // e.g. Attribute("foo", CollectionOf(Bar))

(I know you didn't add them but they don't add much value and where MediaType can be used is already explained below). @deltaskelta fyi - we probably should remove them from the other places that say "wherever...".

Also we should probably change the first line to something like:

// CollectionOf creates a collection media type from its element media type and an optional identifier.
@@ -376,9 +376,17 @@ func Link(name string, view ...string) {
// CollectionOf creates a collection media type from its element media type. A collection media
// type represents the content of responses that return a collection of resources such as "list"
// actions. This function can be called from any place where a media type can be used.
//
// The resulting media type identifier is built from the element media type by appending the media

This comment has been minimized.

@raphael

raphael May 19, 2017

Member

Replace this with:

// If an identifier isn't provided then the resulting media type identifier ...
//
// The collection identifier can be specified as second argument.
//
// CollectionOf(BottleMedia, "vnd.goa.bottles") // Content-Type will be "vnd.goa.bottles".

This comment has been minimized.

@raphael

raphael May 19, 2017

Member

I guess these last 4 lines are not needed with the example above.

@tchssk

This comment has been minimized.

Copy link
Member

tchssk commented May 20, 2017

I added a fixing commit!

@raphael
Copy link
Member

raphael left a comment

Thank you!

@raphael raphael merged commit 4a938b3 into goadesign:master May 20, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@raphael

This comment has been minimized.

Copy link
Member

raphael commented May 20, 2017

Would you mind backporting this to v1? Thank you!

tchssk added a commit to tchssk/goa that referenced this pull request May 21, 2017

Add an argument to CollectionOf() (goadesign#1232)
* Add an argument to CollectionOf()

It makes it possible to specify the collection identifier.

* Fix documents and a test description

raphael added a commit that referenced this pull request May 21, 2017

Add an argument to CollectionOf() (#1232) (#1233)
* Add an argument to CollectionOf()

It makes it possible to specify the collection identifier.

* Fix documents and a test description
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment