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

feat(js): automate with custom templates #3

Merged
merged 5 commits into from
Nov 4, 2021

Conversation

shortcuts
Copy link
Member

@shortcuts shortcuts commented Nov 4, 2021

Notes

  • httpUserAgent does not generate a custom user agent
  • Based on the default typescript-node mustache files
  • Client does not work since the appId is not passed to the hosts

@shortcuts shortcuts changed the base branch from main to feat/js-automation November 4, 2021 11:05
Copy link
Contributor

@bodinsamuel bodinsamuel left a comment

Choose a reason for hiding this comment

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

Not much comments on the generation, seems to work correctly.

The form however is not ideal. I don't have any knowledge about the templates, if we modify anything in the mustache we can no longer be in sync?

.nvmrc Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
algolia-typescript-template/api-single.mustache Outdated Show resolved Hide resolved
algolia-typescript-template/api-single.mustache Outdated Show resolved Hide resolved
algolia-typescript-template/gitignore Show resolved Hide resolved
algolia-typescript-template/package.mustache Outdated Show resolved Hide resolved
app.ts Show resolved Hide resolved
algolia-typescript-template/package.mustache Outdated Show resolved Hide resolved
algolia-typescript-template/package.mustache Outdated Show resolved Hide resolved
algolia-typescript-template/package.mustache Outdated Show resolved Hide resolved
@shortcuts
Copy link
Member Author

@bodinsamuel applied all (I think) your suggestions in ec47092

@shortcuts
Copy link
Member Author

The form however is not ideal. I don't have any knowledge about the templates, if we modify anything in the mustache we can no longer be in sync?

I guess we would have to diff the changes but indeed changes in mustache files are breaking

@damcou
Copy link
Contributor

damcou commented Nov 4, 2021

At this point, do we want to commit on a tool (either openAPI or Swagger codegen) instead of doing the work for both ? (for the POC)

@shortcuts
Copy link
Member Author

shortcuts commented Nov 4, 2021

At this point, do we want to commit on a tool (either openAPI or Swagger codegen) instead of doing the work for both ? (for the POC)

I'm not sure to understand

Edit: got it, sorry! I used OpenAPI since the output and the projects are similar, but I think we should choose one before doing the POC. I guess we could also try the paid feature from Swagger codegen before deciding

@damcou
Copy link
Contributor

damcou commented Nov 4, 2021

At this point, do we want to commit on a tool (either openAPI or Swagger codegen) instead of doing the work for both ? (for the POC)

I'm not sure to understand

Edit: got it, sorry! I used OpenAPI since the output and the projects are similar, but it's up to us to decide which one we should use. I guess we could try the paid feature from Swagger codegen before deciding

Sorry, I was actually looking at the old package.json file locally, now I see that you used openAPI generator, so you can disregard this :| .

Copy link
Contributor

@bodinsamuel bodinsamuel left a comment

Choose a reason for hiding this comment

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

LGTM

(did not check in details the spec itself)

@shortcuts shortcuts marked this pull request as ready for review November 4, 2021 15:12
@shortcuts shortcuts force-pushed the feat/js-client-custom-template branch from ec47092 to 42a5d07 Compare November 4, 2021 15:31
@shortcuts shortcuts changed the base branch from feat/js-automation to main November 4, 2021 15:31
@shortcuts
Copy link
Member Author

oops, it pushed the yarn cache file ._.

@shortcuts
Copy link
Member Author

@damcou @bodinsamuel repo and PR are cleaned, it will be easier to iterate

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