Skip to content

Add option to use swagger documentation as resource type #76

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

Merged
merged 7 commits into from
Jul 24, 2018

Conversation

arojunior
Copy link
Contributor

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
License MIT

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 api-doc-parser as well, including this feature.

src/index.js Outdated
@@ -14,6 +15,7 @@ program
.option('-p, --hydra-prefix [hydraPrefix]', 'The hydra prefix used by the API', 'hydra:')
.option('-g, --generator [generator]', 'The generator to use, one of "react", "react-native", "vue", "admin-on-rest"', 'react')
.option('-t, --template-directory [templateDirectory]', 'The templates directory base to use. Final directory will be ${templateDirectory}/${generator}', `${__dirname}/../templates/`)
.option('-s, --resource-type [resourceType]', 'Hydra or Swagger', 'hydra')
Copy link
Member

Choose a reason for hiding this comment

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

May I suggest -f --format instead?

Should be .option('-f, --format [hydra|swagger]', '"hydra" or "swagger', 'hydra')

src/index.js Outdated
@@ -29,7 +31,14 @@ const generator = generators(program.generator)({
});
const resourceToGenerate = program.resource ? program.resource.toLowerCase() : null;

parseHydraDocumentation(entrypoint).then(ret => {
const parser = entrypoint => {
if (program.resourceType && program.resourceType === 'swagger') {
Copy link
Member

Choose a reason for hiding this comment

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

const parseDocumentation = 'swagger' === program.format ? parseSwaggerDocumentation : parseHydraDocumentation;

@arojunior
Copy link
Contributor Author

@dunglas It's done. Can you review again? Thanks.

@arojunior
Copy link
Contributor Author

Can I do something else to get this PR accepted?

@dunglas
Copy link
Member

dunglas commented Jun 24, 2018

Hi @arojunior, thank you very much for working on this, we definitely want to merge this PR but we can't do it if the tests are red.
Can you try to fix them? https://travis-ci.org/api-platform/client-generator/jobs/390537974

For the CS problems, you can just run yarn fix. For the deps error, do I need to tag a new version of API doc parser?

@arojunior
Copy link
Contributor Author

@dunglas ok, thanks. I made the fixes with yarn fix. About the error with deps, I need your help in this issue

@arojunior arojunior closed this Jul 9, 2018
@arojunior arojunior deleted the swagger-resource branch July 9, 2018 12:20
@arojunior arojunior restored the swagger-resource branch July 9, 2018 12:21
@arojunior arojunior reopened this Jul 9, 2018
@arojunior
Copy link
Contributor Author

Hey @dunglas, thanks a lot for your help. Now everything is OK and tests are green. Can you merge the PR?

@dunglas
Copy link
Member

dunglas commented Jul 9, 2018

The code looks good to me! Thank you very much for working on this.
Can you add some tests (maybe similar to those ones: https://github.com/api-platform/client-generator/blob/master/package.json#L53-L54) to be prevent regressions?

@arojunior
Copy link
Contributor Author

@dunglas is it ok now?

@arojunior
Copy link
Contributor Author

@dunglas should I do something else?

@dantaub
Copy link

dantaub commented Jul 23, 2018

Is this going to be merged soon? I'd like to try to use this package with my swagger API definitions... Alternatively, anyone have tips on how to yarn/npm install from @arojunior's branch? The namespacing seems to keep it from building after it's downloaded..

@dunglas dunglas merged commit 74c5b08 into api-platform:master Jul 24, 2018
@dunglas
Copy link
Member

dunglas commented Jul 24, 2018

Thanks @arojunior

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.

3 participants