Skip to content

Swagger parser#30

Merged
dunglas merged 4 commits intoapi-platform:masterfrom
arojunior:swagger-parser
Jun 5, 2018
Merged

Swagger parser#30
dunglas merged 4 commits intoapi-platform:masterfrom
arojunior:swagger-parser

Conversation

@arojunior
Copy link
Copy Markdown
Contributor

@arojunior arojunior commented May 13, 2018

The work still in progress, but I'm open this Pull Request just to let you guys know how it is going.

I'm open a PR to client-generator as well, including this feature.

}

/**
* Parses a Hydra documentation and converts it to an intermediate representation.
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

On a quick scan i noticed this mentions Hydra, should be swagger i expect.

@arojunior
Copy link
Copy Markdown
Contributor Author

I think now it's OK. Can you guys make a review?

Copy link
Copy Markdown
Member

@dunglas dunglas left a comment

Choose a reason for hiding this comment

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

Awesome! Thank you very much for working on this. It's definitely something we want in.

Comment thread src/swagger/handleJson.js Outdated
import Field from "../Field";
import Resource from "../Resource";

String.prototype.capitalize = function() {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's confusing because it doesn't do the same thing than underscore/lodash capitalize.

Can you use _.upperFirst() instead of adding new functions to the global String prototype? We already use lodash in the project: https://github.com/api-platform/api-doc-parser/blob/master/package.json#L39

Comment thread src/swagger/handleJson.js Outdated
export default function(response, entrypointUrl) {
const paths = Object.keys(response.paths)
.map(item => item.replace(`/{id}`, ``))
.filter(onlyUnique);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Comment thread src/swagger/handleJson.js Outdated
const fields = fieldNames.map(
fieldName =>
new Field(fieldName, {
id: `http://schema.org/${fieldName}`,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

By definition, we cannot guess the Schema.org mapping from a Swagger file (JSON-LD must be used for this). The id should always be null when using Swagger.

It's maybe possible to add such mappings with OpenAPI v3, I've not checked.

Comment thread src/swagger/handleJson.js
fieldName =>
new Field(fieldName, {
id: `http://schema.org/${fieldName}`,
range: null,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It should be possible to map Swagger types to a RDF ranges. Can be done in a second step anyway.

Comment thread src/swagger/handleJson.js
new Field(fieldName, {
id: `http://schema.org/${fieldName}`,
range: null,
reference: null,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It should be possible to guess if this field is a relation or not using Swagger. Can be done in a second step anyway.

Comment thread src/swagger/handleJson.js Outdated
);

return new Resource(name, url, {
id: `http://schema.org/` + title,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same here, it cannot start with http://schema.org/ (should be null IMO)

@dunglas
Copy link
Copy Markdown
Member

dunglas commented May 29, 2018

Have you tried to use your branch with admin or client-generator to see if it works well?

@arojunior
Copy link
Copy Markdown
Contributor Author

Hi @dunglas, thank you for your time reviewing my PR. I just made some adjusts as you asked.

About the client-generator... Yes, I tried. My PR there is already tested.

@dunglas dunglas merged commit 0c999c9 into api-platform:master Jun 5, 2018
@dunglas
Copy link
Copy Markdown
Member

dunglas commented Jun 5, 2018

Thanks @arojunior

jfthuillier pushed a commit to jfthuillier/api-doc-parser that referenced this pull request Jun 15, 2018
* add support to parse Swagger format

* eslint fixes

* adjusts required by revision to swagger parser
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.

4 participants