-
Notifications
You must be signed in to change notification settings - Fork 6
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(progress-bar): setup basic #98
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: flakey5 <[email protected]>
Co-authored-by: flakey5 <[email protected]>
Co-authored-by: flakey5 <[email protected]>
@AugustinMauroy, any progress here? Would love to have this landed! |
I'll take time today to fix this pr. When a generator as ended the process still running because the progressbar did't ended correctly |
@ovflowd could you give last review ? 😄 |
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.
IMO the progress bar should be for:
- Loader (progress of loading files)
- Parser (progress of parsing files)
- Top Level generator.mjs (progress of going through generators, instead of each file being generated)
src/loader.mjs
Outdated
progressBar.increment(); | ||
// normally we stop the progress bar when the loop is done | ||
// but here we return the loop so we need to stop it when the last file is loaded | ||
if (progressBar.value === progressBar.total) { |
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.
Is the .stop() call needed? what happens if we simply stop incrementing when value === total?
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.
sometimes it's let the node process running for any reasons. So yeah I think it's needed to call .stop()
method.
But it's can change if we use clack
src/utils/generators.mjs
Outdated
@@ -12,14 +14,20 @@ export const groupNodesByModule = nodes => { | |||
/** @type {Map<string, Array<ApiDocMetadataEntry>>} */ | |||
const groupedNodes = new Map(); | |||
|
|||
const progressBar = createProgressBar(groupNodesByModule.name); | |||
progressBar.start(nodes.length, 0); |
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 this should be inside this util :)
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.
To clarify the only thing this util should do is to group nodes by module; This is breaking the single responsibility pattern.
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.
Humm yeah
Bump, @AugustinMauroy |
IMO there should be a way to disable the progress bar, like |
+1 would that be a |
BTW @AugustinMauroy wdyt about https://github.com/bombshell-dev/clack feels like a nicer way for us to design this and looks neat! |
I need to take a look before giving my opinion |
Okay first look of clack, This may be an alternative to ordering, but the package does not have a progress bar feature. But it does have a spinner, which doesn't really help you keep track of the process. |
Shouldn't it be the other way round? Shouldn't there be a flag displaying the progressbar? |
Well you can always render:
We don't necessarily need a progress bar, but a way to track that there's progress. |
I don't care either way, I just think there should be a toggle to disable/enable it. I don't care if it's toggle on or toggle off |
I think it should be showing progress by default and an option to not show progress. Otherwise it doesn't make any sense lol |
okay I'll try to do something like that: But note:
|
That is just an example file, you can simply paste that here :) |
BTW @AugustinMauroy is there any update here? |
okay I use clack now. I din't have tested yet |
Description
Adding a cool progress bar to know what is going on.
Validation
Run this command and see progress bar
npx api-docs-tooling -i "/path/to/node/doc/api/*.md" -t legacy-html -o /tmp/out
Related Issues
Close #58
Check List