-
Notifications
You must be signed in to change notification settings - Fork 6
Enhancement/generate query params #590
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall, only the package-lock.json
needs to be removed.
Have you tried to use openapi-generator templates? I think they are quite powerfull, I had a branch where I did some testing, I've created a pull request with the changes #592
I think is a better approach since it also generates all the enums for autocompletion and validation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please review AI generated code before submitting, this file needs to be removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Despite my review, this was not intended to be committed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The scripts
folder is excluded from eslint, maybe we should add it so we have consistent styling
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better still might be to create scripts/
directory in generated-client
, though we would have to exempt that directory from being ignored by eslint/tsc as well, since as I recall, that parent directory and its children are ignored.
@@ -0,0 +1,98 @@ | |||
|
|||
|
|||
import * as fs from 'fs'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import * as fs from 'fs'; | |
import { readFileSync, writeFileSync } from 'fs'; |
|
||
|
||
import * as fs from 'fs'; | ||
import * as yaml from 'js-yaml'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import * as yaml from 'js-yaml'; | |
import { load } from 'js-yaml'; |
|
||
import * as fs from 'fs'; | ||
import * as yaml from 'js-yaml'; | ||
import * as path from 'path'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import * as path from 'path'; | |
import { resolve } from 'path'; |
.map((part) => part.charAt(0).toUpperCase() + part.slice(1)) | ||
.join(''); | ||
|
||
return `${method.charAt(0).toUpperCase() + method.slice(1)}${pascalCaseName}QueryParams`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only GET
methods are processed, what do you think on removing the method from the name? This also applies for the QueryParams
suffix
return `${method.charAt(0).toUpperCase() + method.slice(1)}${pascalCaseName}QueryParams`; | |
return pascalCaseName; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, the names are terrible; QueryParams is fine for the type name and is not needed here
function pathToTypeName(method: string, apiPath: string): string { | ||
const cleanedPath = apiPath | ||
.replace(/[^a-zA-Z0-9/_{}]/g, '') // Remove special characters except slashes and braces | ||
.replace(/{/g, 'By/') // Replace { with By/ for path parameters |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we care about this? I think it makes it a bit hard to read, for example GetOsidbApiV1FlawsByFlaw_idCvss_scoresByIdQueryParams
, this same method is generated with the name osidbApiV1FlawsCommentsRetrieve
by the openapi-generator
} | ||
|
||
const outputContent = | ||
`// This file is auto-generated by scripts/get-osidb-query-params.ts\n// Do not edit this file manually.\n\n${typeStrings.join('\n')}\n`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would probably add also an exclude for linter tools, we don't want this to prevent merging a pull request if for some reason it fails
/* tslint:disable */
/* eslint-disable */
|
||
export async function getAffects(cveOrUuid: string) { | ||
const field = isCveValid(cveOrUuid) ? 'cve_id' : 'flaw__uuid'; | ||
const field: GetOsidbApiV1AffectsQueryParams = isCveValid(cveOrUuid) ? 'flaw__cve_id' : 'flaw__uuid'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The OSIDB pull request has been merged already, update the openapi schema and regenerate the types to use the new cve_id
field
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just a proof of concept, we can do so in another PR
I explored this briefly, but I was targeting a smaller footprint for the proof of concept. If we want to define our own template and if that PR implementation limits the amount of manual effort to intervene for schema changes then I would prefer your implementation. |
Generates query params from OpenAPI yml!