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

Document reusing schemas #244

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Document reusing schemas #244

wants to merge 2 commits into from

Conversation

char0n
Copy link

@char0n char0n commented Jan 3, 2020

Closes #243

@char0n
Copy link
Author

char0n commented Jan 3, 2020

This still doesn't work correctly. E.g. when we reference Errors schema, only this schema is included in generated swagger, but we don't know how Error schema look like, because from some reason it's not included.

OK hopefully got it right now.

Copy link
Contributor

@mbuhot mbuhot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for writing this up! 🙏

docs/reusing-schemas.md Outdated Show resolved Hide resolved
use PhoenixSwagger

@doc """
Returns map of common swagger definitions merged with the map of provided schema definitions.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These very general schemas could be added to the MyAppWeb.Router.swagger_info callback.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you pls elaborate more or provide a concrete code example ?


@doc false
def swagger_definitions do
create_swagger_definitions(%{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is a simpler way to include specific schemas by putting each shared schema in its own module and including it in the map:

%{
  ListOfProjects: ... schema definitions ...
  Address: MyAppWeb.Schemas.Adress.schema()
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes this is something that I did from be beginning. But the problem was that along with Address you have to include all other general common schemas, that Address schema is using. Otherwise they're not gonna appear in generate swagger.json.

end
```

To avoid importing `create_swagger_definitions/1` in every controller, find a `HelloWeb` module and add an import
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This advice feels a bit opinionated. Users are free to organise their projects and code as they like. My preference is to delete this file for API projects trading off some dry-ness for explicitness of imports.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I agree, this is my opinionated hint how to use something that pre-generated phoenix project already contains. Should we remove this section then ?

parameters do
sort_by :query, :string, "The property to sort by"
sort_direction :query, :string, "The sort direction", enum: [:asc, :desc], default: :asc
company_id :string, :query, "The company id"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why does location and type differ between line 28 & 29?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The example was directly copied from this location of the official documentation: https://hexdocs.pm/phoenix_swagger/reusing-swagger-parameters.html#content

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.

Sharing common definitions
3 participants