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

feat(progress-bar): setup basic #98

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

Conversation

AugustinMauroy
Copy link
Member

@AugustinMauroy AugustinMauroy commented Sep 11, 2024

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

  • I have read the Contributing Guidelines and made commit messages that follow the guideline.
  • I've covered new added functionality with unit tests if necessary.

src/loader.mjs Outdated Show resolved Hide resolved
src/loader.mjs Outdated Show resolved Hide resolved
AugustinMauroy and others added 3 commits September 12, 2024 09:05
Co-authored-by: flakey5 <[email protected]>
Co-authored-by: flakey5 <[email protected]>
Co-authored-by: flakey5 <[email protected]>
@ovflowd
Copy link
Member

ovflowd commented Oct 14, 2024

@AugustinMauroy, any progress here? Would love to have this landed!

@AugustinMauroy
Copy link
Member Author

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

@AugustinMauroy AugustinMauroy marked this pull request as ready for review October 14, 2024 19:27
@AugustinMauroy AugustinMauroy requested a review from a team as a code owner October 14, 2024 19:27
@AugustinMauroy
Copy link
Member Author

@ovflowd could you give last review ? 😄

src/utils/progressBar.mjs Outdated Show resolved Hide resolved
Copy link
Member

@ovflowd ovflowd left a 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:

  1. Loader (progress of loading files)
  2. Parser (progress of parsing files)
  3. Top Level generator.mjs (progress of going through generators, instead of each file being generated)

src/loader.mjs Outdated Show resolved Hide resolved
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) {
Copy link
Member

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?

Copy link
Member Author

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/parser.mjs Outdated Show resolved Hide resolved
@@ -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);
Copy link
Member

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 :)

Copy link
Member

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Humm yeah

@ovflowd
Copy link
Member

ovflowd commented Nov 18, 2024

Bump, @AugustinMauroy

@avivkeller
Copy link
Member

IMO there should be a way to disable the progress bar, like --no-progress or something.

@ovflowd
Copy link
Member

ovflowd commented Nov 18, 2024

IMO there should be a way to disable the progress bar, like --no-progress or something.

+1 would that be a --quiet mode (?) not sure we have any other sort of output

@ovflowd
Copy link
Member

ovflowd commented Nov 18, 2024

BTW @AugustinMauroy wdyt about https://github.com/bombshell-dev/clack feels like a nicer way for us to design this and looks neat!

@AugustinMauroy
Copy link
Member Author

BTW @AugustinMauroy wdyt about https://github.com/bombshell-dev/clack

I need to take a look before giving my opinion

@AugustinMauroy
Copy link
Member Author

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.

@AugustinMauroy
Copy link
Member Author

+1 would that be a --quiet mode (?) not sure we have any other sort of output

Shouldn't it be the other way round? Shouldn't there be a flag displaying the progressbar?

@ovflowd
Copy link
Member

ovflowd commented Nov 18, 2024

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.

Well you can always render:

:spinner: (1/40) - Generating: (module that it is generating)

We don't necessarily need a progress bar, but a way to track that there's progress.

@avivkeller
Copy link
Member

+1 would that be a --quiet mode (?) not sure we have any other sort of output

Shouldn't it be the other way round? Shouldn't there be a flag displaying the progressbar?

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

@ovflowd
Copy link
Member

ovflowd commented Nov 18, 2024

+1 would that be a --quiet mode (?) not sure we have any other sort of output

Shouldn't it be the other way round? Shouldn't there be a flag displaying the progressbar?

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

@AugustinMauroy
Copy link
Member Author

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.

Well you can always render:

:spinner: (1/40) - Generating: (module that it is generating)

We don't necessarily need a progress bar, but a way to track that there's progress.

okay I'll try to do something like that:
https://github.com/bombshell-dev/clack/blob/main/examples/basic/spinner.ts

But note:

  • This package isn't tested
  • This package didn't publish npm release with npm provenance (what cli-progress doesn't do either)
  • Not yet in 1.0.0 and very recent

@ovflowd
Copy link
Member

ovflowd commented Dec 19, 2024

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.

Well you can always render:

:spinner: (1/40) - Generating: (module that it is generating)

We don't necessarily need a progress bar, but a way to track that there's progress.

okay I'll try to do something like that: bombshell-dev/clack@main/examples/basic/spinner.ts

But note:

  • This package isn't tested
  • This package didn't publish npm release with npm provenance (what cli-progress doesn't do either)
  • Not yet in 1.0.0 and very recent

That is just an example file, you can simply paste that here :)

@ovflowd
Copy link
Member

ovflowd commented Dec 19, 2024

BTW @AugustinMauroy is there any update here?

@AugustinMauroy
Copy link
Member Author

okay I use clack now. I din't have tested yet

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.

Add cli-progress (Progress Bar) for CLI
4 participants