-
Notifications
You must be signed in to change notification settings - Fork 43
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(openapi): adding a command to resolve circular and recursive references #1063
base: next
Are you sure you want to change the base?
feat(openapi): adding a command to resolve circular and recursive references #1063
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 great so far! haven't had a chance to properly test this yet, but some preliminary feedback below! also if you could add some docs in README.md
that'd be great. thanks!
src/cmds/openapi/refs.ts
Outdated
return 'File path is required.'; | ||
} | ||
|
||
const openApiData = OpenAPISolvingCircularityAndRecursiveness.readOpenApiFile(spec); |
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.
rather than using this static method to read the file, can you use our existing prepareOas
function? see an example here:
rdme/src/cmds/openapi/convert.ts
Line 57 in 5db25a1
const { preparedSpec, specPath, specType } = await prepareOas(spec, 'openapi:convert', { convertToLatest: true }); |
src/cmds/openapi/refs.ts
Outdated
} | ||
} | ||
|
||
OpenAPISolvingCircularityAndRecursiveness.writeOpenApiFile(spec, openApiData); |
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.
rather than overwriting the existing file, can you add a prompt (and a corresponding --out
flag) so users can specify what the updated file path should be? see here for an example:
rdme/src/cmds/openapi/convert.ts
Lines 66 to 85 in 5db25a1
prompts.override({ | |
outputPath: opts.out, | |
}); | |
const promptResults = await promptTerminal([ | |
{ | |
type: 'text', | |
name: 'outputPath', | |
message: 'Enter the path to save your converted/bundled API definition to:', | |
initial: () => { | |
const extension = path.extname(specPath); | |
return `${path.basename(specPath).split(extension)[0]}.openapi${extension}`; | |
}, | |
validate: value => validateFilePath(value), | |
}, | |
]); | |
Command.debug(`saving converted/bundled spec to ${promptResults.outputPath}`); | |
fs.writeFileSync(promptResults.outputPath, JSON.stringify(parsedPreparedSpec, null, 2)); |
…rity-and-recursiveness
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.
DM'd you about this but this has a few merge conflicts now with the changes that were shipped in #1068 — happy to help with these if you need a hand!
…napi-adding-command-to-solve-circularity-and-recursiveness
…cursiveness' of https://github.com/readmeio/rdme into oleh/openapi-adding-command-to-solve-circularity-and-recursiveness
…rity-and-recursiveness
…rity-and-recursiveness
…rity-and-recursiveness
…rity-and-recursiveness
…cursiveness' of https://github.com/readmeio/rdme into oleh/openapi-adding-command-to-solve-circularity-and-recursiveness
…rity-and-recursiveness
…rity-and-recursiveness
…rity-and-recursiveness
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.
more feedback! i'll try and open a PR this week based off of yours with copy edits
import { runCommandAndReturnResult } from '../../helpers/oclif.js'; | ||
|
||
describe('openapi:solving-circularity-recursiveness', () => { | ||
let run: (args?: string[]) => Promise<unknown>; |
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.
let run: (args?: string[]) => Promise<unknown>; | |
let run: (args?: string[]) => Promise<string>; |
src/commands/openapi/refs.ts
Outdated
}, | ||
{ | ||
description: 'If you wish to automate this command, you can pass in CLI arguments to bypass the prompts:', | ||
command: '<%= config.bin %> <%= command.id %> petstore.json -out petstore.openapi.json', |
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.
command: '<%= config.bin %> <%= command.id %> petstore.json -out petstore.openapi.json', | |
command: '<%= config.bin %> <%= command.id %> petstore.json --out petstore.openapi.json', |
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.
can you revert this change?
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 returned this file, there was some confusion with them(
src/commands/openapi/refs.ts
Outdated
* @param {OASDocument} document - The OpenAPI document to analyze. | ||
* @returns {Promise<string[]>} A list of circular reference paths. | ||
*/ | ||
static async getCircularRefsFromOas(document: OASDocument): Promise<string[]> { |
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.
can you go through and remove all of the type definitions that you have in JSDocs (e.g., @param {OASDocument}
) and move your parameter docs above the parameters themselves? they're superfluous since you're already are defining the types in your TS function parameter type annotations. see below for an example!
* @param {OASDocument} document - The OpenAPI document to analyze. | |
* @returns {Promise<string[]>} A list of circular reference paths. | |
*/ | |
static async getCircularRefsFromOas(document: OASDocument): Promise<string[]> { | |
* @returns A list of circular reference paths. | |
*/ | |
static async getCircularRefsFromOas( | |
/** The OpenAPI document to analyze. */ | |
document: OASDocument, | |
): Promise<string[]> { |
src/commands/openapi/refs.ts
Outdated
|
||
if (remainingCircularRefs.length > 0) { | ||
// eslint-disable-next-line no-console | ||
console.info( |
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.
can you use the info
logger from lib/logger
instead?
Line 42 in a55dd14
function info( |
## 🧰 Changes Touched up a few things in #1063! - [x] Documentation copy edits - [x] Logging touch ups - [x] Makes sure we're only allowing this to be run against OpenAPI - [ ] Adds a stricter type in 81607ca that ends up breaking the build — I believe this is a code smell. @olehshh can you look into fixing the type errors here? ## 🧬 QA & Testing Are my changes sound?
…d-to-solve-circularity-and-recursiveness
This reverts commit 9249d2a.
…rity-and-recursiveness
…cursiveness' of https://github.com/readmeio/rdme into oleh/openapi-adding-command-to-solve-circularity-and-recursiveness
…rity-and-recursiveness
…rity-and-recursiveness
…rity-and-recursiveness
…rity-and-recursiveness
🧰 Changes
🧬 QA & Testing
Take any file with circular references, for example
Execute the
rdme openapi:refs [path]
command and useinspect
to check for circular references after execution. The command also handles recursiveness.