-
-
Notifications
You must be signed in to change notification settings - Fork 60
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: automatically generate docs #318
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.
Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide useful for opening a pull request.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.
Pull Request Test Coverage Report for Build 3893317675Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
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 to me, we are only missing a test to ensure it works 🙂 There is a bunch of middleware test you can look for guidance, otherwise let me know if you get stuck or have any questions 🙂
@jonaslagoni please kindly review this PR :) |
} | ||
|
||
expect(err).toBeUndefined(); | ||
fs.emptyDirSync(testDir); |
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 at the end remove the testDir
dir, not only remove content from it.
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.
@magicmatatjahu what do you mean by "not only remove content from it"?
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.
Read that https://github.com/jprichardson/node-fs-extra/blob/master/docs/emptyDir-sync.md. You should remove dir.
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.
Ok.. my fault. You shouldn't. Don't focus on my previous comments.
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.
Okay 😂. Is the PR okay?
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 don't know if PR is okay, but that test is.
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.
Left a couple of comments. I think this PR could be simpler. If I'm understanding correctly, you don't need the readGleeConfig
util function. We already have the initializeConfig
function that does the same and it's actually doing it with the config only.
src/index.ts
Outdated
@@ -41,7 +44,7 @@ export default async function GleeAppInitializer () { | |||
showAppDir: GLEE_PROJECT_DIR !== process.cwd(), | |||
showFunctionsDir: GLEE_FUNCTIONS_DIR !== resolve(GLEE_DIR, 'functions'), | |||
}) | |||
|
|||
const gleeDocs = await readGleeConfig(GLEE_CONFIG_FILE_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.
I don't think you need the readGleeConfig
function. You already have the config loaded a few lines before.
src/middlewares/generateDocs.ts
Outdated
) | ||
return 'done' | ||
} catch (error) { | ||
logLineWithIcon( |
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.
You can use this instead: https://github.com/asyncapi/glee/blob/master/src/lib/logger.ts#L123.
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.
@fmvilas I just updated the PR. Kindly review it :)
src/index.ts
Outdated
@@ -120,6 +121,7 @@ export default async function GleeAppInitializer () { | |||
}) | |||
|
|||
app.on('adapter:server:ready', (e: EnrichedEvent) => { | |||
generateDocs(parsedAsyncAPI, config, null) |
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.
Why here? If we generate docs on "adapter:server:ready", it will only work for WebSocket and HTTP servers but not for clients (MQTT, Kafka, etc.). I think it should be generated as soon as possible and not necessarily when an event happens.
src/middlewares/generateDocs.ts
Outdated
await generator.generateFromString(JSON.stringify(resolvedData)) | ||
logLineWithIcon( | ||
':zap:', | ||
'Successfully generated docs for your specification' |
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.
"specification" = https://github.com/asyncapi/spec/blob/master/spec/asyncapi.md
"document" = https://github.com/asyncapi/spec/blob/master/examples/simple.yml
People don't have specs but AsyncAPI documents. I'd keep it simpler and rephrase it as:
'Successfully generated docs for your specification' | |
'Successfully generated docs' |
src/middlewares/generateDocs.ts
Outdated
logErrorLine( | ||
`Failed to generate docs for your specs due to the following reason: ${error}`, | ||
) |
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.
If you're going to log the reason, i.e., the full error, I recommend you use logError. Have a look at the logger.ts
file and get familiar with the stuff there 😄
logErrorLine( | |
`Failed to generate docs for your specs due to the following reason: ${error}`, | |
) | |
logErrorLine('Failed to generate docs...') | |
logError(error) |
test/lib/util.test.ts
Outdated
@@ -49,4 +51,27 @@ describe('util', () => { | |||
expect(functionEvent.glee).toBe(glee) | |||
}) | |||
}) | |||
describe('readGleeConfig', () => { |
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.
🚜 ...
😄
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.
@fmvilas I also didn't get the message you're trying to pass here 😂
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.
hahaha I knew this can happen but I like to comment like this when something needs to be removed
😄 We're not using this function anymore, right?
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.
Yes, you're right :)
it('should generate documentation', async () => { | ||
const testDir = tmpdir() + `/${CONFIG_TEST_DATA.generator.folder}`; | ||
fs.emptyDirSync(testDir); | ||
let err: Error | undefined = undefined; |
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 think you can do something like this:
let err: Error | undefined = undefined; | |
let err?: Error |
or like this, I can't remember:
let err: Error | undefined = undefined; | |
let err: Error? |
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.
@fmvilas I don't think I understand what you're trying to do here but it seems your code snippet is wrong :)
Mind explaining what you're trying to achieve with that snippet?
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 was trying to do something like this: https://www.typescriptlang.org/docs/handbook/advanced-types.html#optional-parameters-and-properties. But it looks like it only works on parameters and object properties 🤦
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.
In any case, you can simplify it like this:
let err: Error | undefined = undefined; | |
let err: Error | undefined |
src/lib/docs.ts
Outdated
|
||
export default async (spec, config, resDir) => { | ||
const configData = config.docs | ||
if (configData?.enabled) { |
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.
With this check, docs will only be generated if we add this configuration to the config file. It would be awesome if, by default, it generated docs and then people can disable it if they want to. So maybe it should be configData?.disabled
instead? Folder can default to docs
and template to @asyncapi/markdown-template
.
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.
Why should it generate if it says configData?.disabled
? isn't that saying generate if disabled?
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.
shouldn't i be doing !configData || configData?.enabled
?
src/lib/docs.ts
Outdated
|
||
export default async (spec, config, resDir) => { | ||
const configData = config.docs | ||
if (!configData || configData?.enabled) { |
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.
Something like this @fmvilas
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.
What about the case when configData
exists but enabled
is not defined? It should still work but with this check it won't. The reason I said to use disabled
instead is that you can easily check for it like this:
if (configData?.disabled) return
This way, you early return only when you find that disabled
is true.
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.
Alternatively, you can check for this:
if (configData?.enabled === false) return
That should work as well, I think.
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.
But seems more logical to me that, since the default is that it generates docs, you only go there when you want to disable it or when you want to change the template/folder.
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.
Ah! Now I understand your point @fmvilas
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.
Updated the PR @fmvilas. wyt?
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.
LGTM! left some suggestions.
docs/config-file.md
Outdated
enabled: true // Enable/Disable documentation generation | ||
folder: 'docs' // Folder where you want the output of your docs to reside. | ||
template: '@asyncapi/markdown-template' // Type of template you want to use. | ||
} |
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.
enabled: true // Enable/Disable documentation generation | |
folder: 'docs' // Folder where you want the output of your docs to reside. | |
template: '@asyncapi/markdown-template' // Type of template you want to use. | |
} | |
enabled: true, // Enable/Disable documentation generation | |
folder: 'docs', // Folder where you want the output of your docs to reside. | |
template: '@asyncapi/markdown-template' // Type of template you want to use. | |
} |
package.json
Outdated
@@ -36,6 +36,8 @@ | |||
"license": "Apache-2.0", | |||
"dependencies": { | |||
"@asyncapi/generator": "^1.9.3", |
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 we safely bump up the generator version here? 🤔 getting some incompatibility errors with this version.
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.
Thanks for the review @KhudaDad414
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.
🚢 🙇
Pinging you all for visibility @fmvilas @KhudaDad414 @Souvikns |
Kudos, SonarCloud Quality Gate passed!
|
Updated the ReadME @fmvilas. wyt? |
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.
LGTM 👍
@jonaslagoni will have to approve too. |
/rtm |
🎉 This PR is included in version 0.13.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Description
This PR provides a way to automatically generate docs
It generates docs in a folder inside the project. Fully configurable from the glee.config.js file, like the path to the folder and the Generator template to use. We can default to docs and the https://github.com/asyncapi/markdown-template/.
Related issue(s)
Resolves #261