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 10 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 ? 😄

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)

@@ -26,8 +28,17 @@ const createLoader = () => {
filePath => extname(filePath) === '.md'
);

const progressBar = createProgressBar('Loading files');
progressBar.start(resolvedFiles.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.

Can you add white spaces between statements and if conditions?

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?

const resolvedApiDocEntries = await Promise.all(
apiDocs.map(async apiDoc => {
progressBar.increment();
return await parseApiDoc(apiDoc);
Copy link
Member

Choose a reason for hiding this comment

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

No need of adding return await the function itself is already async, so you can drop the async apiDoc and simply do return parseApiDoc

Since the map function doesn't do any async work itself.

@@ -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.

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
3 participants