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

Support swagger extensions #779

Merged
merged 7 commits into from Sep 19, 2016

Conversation

@tchssk
Copy link
Member

commented Sep 16, 2016

This pull request extends apidsl.Metadata() to support Swagger Extensions.
Now it includes only core implementations so I will add tests and comments for godoc.

https://gophers.slack.com/archives/goa/p1473747445000640

@raphael
Copy link
Member

left a comment

Thank you for putting this together, this looks great! Just add the missing comments so the linter is happy. Also it would be great if there were some tests. Thank you!

}

_Info Info

This comment has been minimized.

Copy link
@raphael

raphael Sep 16, 2016

Member

Can you leave a comment explaining why the _ types are needed?

return merged, nil
}

func (i Info) MarshalJSON() ([]byte, error) {

This comment has been minimized.

Copy link
@raphael

raphael Sep 16, 2016

Member

Please add a comment for these

@tchssk tchssk force-pushed the tchssk:swagger-extensions branch from 17b6142 to 4f91870 Sep 16, 2016

@tchssk

This comment has been minimized.

Copy link
Member Author

commented Sep 16, 2016

Oops, I added the comments but travis tells me another error...

@raphael

This comment has been minimized.

Copy link
Member

commented Sep 17, 2016

Pro tip: run make before pushing, that's what Travis does :)

@tchssk

This comment has been minimized.

Copy link
Member Author

commented Sep 17, 2016

Thanks. I forget to make sometimes😓

@tchssk tchssk force-pushed the tchssk:swagger-extensions branch from 1f4526a to 41eddae Sep 18, 2016

@raphael

This comment has been minimized.

Copy link
Member

commented Sep 19, 2016

Looks great, thank you for the awesome PR! Is it ready for merge? (LGTM)

@tchssk tchssk force-pushed the tchssk:swagger-extensions branch from ba57024 to 9b6a2bb Sep 19, 2016

@tchssk

This comment has been minimized.

Copy link
Member Author

commented Sep 19, 2016

Thank you for your waiting. It is ready.

@raphael

This comment has been minimized.

Copy link
Member

commented Sep 19, 2016

Do you think you could also send a PR against the v1 branch? this is a backwards compatible change and it would be great if people on the v1 branch could benefit from it too :)

@raphael raphael merged commit 61b3b6e into goadesign:master Sep 19, 2016

@tchssk

This comment has been minimized.

Copy link
Member Author

commented Sep 19, 2016

It's OK. But this PR is based on #775 so I will send that to v1 first.

@tchssk tchssk referenced this pull request Sep 19, 2016
@raphael

This comment has been minimized.

Copy link
Member

commented Sep 19, 2016

Perfect, thanks again!

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