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

Added support for injecting MD files. Any extension starts with 'x-md… #327

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

TomerG711
Copy link

…-' will be treated as md file path, and loaded into 'md_' property. The content of the MD file can be accessed in the templates.

This will allow us to create much more complex MD files, making Widdershins much more flexible.

…-*' will be treated as md file path, and loaded into 'md_*' property. The content of the MD file can be accessed in the templates.
Copy link
Contributor

@MikeRalphson MikeRalphson left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! In addition to the minor comments, I have some questions on the approach. Where in the input definition can x-md- extensions appear, is it only on paths and tags? I think it would be more generally useful if the approach could be widened a bit, so that it applied equally well to AsyncAPI (i.e. was moved to lib/common.js) and we found some way to deepen the ability to attach the extensions to more points in the input.

We would also need documentation changes and potentially a switch to control this / control the extension prefix.

lib/openapi3.js Outdated
for (let field in sourceSection) {
if (field.startsWith("x-md-")) {
//Replacing '-' so that field can be used in dot templates
let newFieldName = field.replace("x-md-", "md_");
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we replace all - with _ not just the first two? Either with a split/join or a .replace.

lib/openapi3.js Outdated
if (field.startsWith("x-md-")) {
//Replacing '-' so that field can be used in dot templates
let newFieldName = field.replace("x-md-", "md_");
destSection[newFieldName] = await fs.readFile(sourceSection[field], 'utf8');
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing error handling on the readFile.

lib/openapi3.js Outdated
@@ -104,8 +124,7 @@ function fakeProdCons(data) {
let schema = op.requestBody.content[rb].schema;
if (schema['x-widdershins-oldRef']) {
data.bodyParameter.refName = schema['x-widdershins-oldRef'].replace('#/components/schemas/', '');
}
else {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the first of many unrelated formatting changes even with GitHub diff set to ignore whitespace changes. Could we lose these please?

@TomerG711
Copy link
Author

@MikeRalphson I've made the changes you asked. I added support to loading MD everywhere extensions are allowed, according to: https://swagger.io/docs/specification/openapi-extensions/.
Also made the field prefix configurable, and updated README.

@TomerG711
Copy link
Author

@MikeRalphson Could you please take a look?

@MikeRalphson
Copy link
Contributor

@TomerG711 thanks for your patience. I'm evaluating a slightly different solution whereby we simply recurse through the input API definition (whether it is OpenAPI or AsyncAPI or anything else) and update the x-md- (or whatever prefix is specified) extensions - using code similar to what you have in the PR now.

Regardless of which approach we go with, I think we would need to support fetching of resources which are http(s) links or are relative to API definitions loaded from a URL.

lib/common.js Outdated Show resolved Hide resolved
@TomerG711
Copy link
Author

So should I add support for fetching remote resources or should I leave it for you with the other solution?

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.

2 participants