ljharb / repo-report

CLI to list all repos a user has access to, and report on their configuration in aggregate.
MIT License
24 stars 11 forks source link

add loading logs while fetching repositories #81

Closed abdumamdouh closed 2 years ago

abdumamdouh commented 2 years ago

Fixes #80

abdumamdouh commented 2 years ago

I think i'd really prefer to look into some kind of approach where we print to the console, but overwrite it - using process.stdout.write, perhaps?

Either way, we shouldn't be logging inside these reusable functions, and definitely not inside getRepositories

can you more explain about process.stdout.write and where exactly can we logging the loading indicator

ljharb commented 2 years ago

The analogy i have in my head is printf vs echo. On a shell, you can use printf to print characters on a line without moving to the next line, which means you can rewrite that line later.

abdumamdouh commented 2 years ago

@ljharb we have used stdout instead of console log

i think it's ready now to be merged

KhatiaIvanova commented 2 years ago

@ljharb Do we need to update something more?

ljharb commented 2 years ago

Let me give it a whirl locally :-)

ljharb commented 2 years ago

ok, this technique is awesome!

I think we need a bit more polish though:

I don't think this should happen inside getRepositories - I think we should have a shared function that's used in both src/commands/detail.js and src/commands/ls.js, something like this:

async function loadingIndicator(task) {
  process.stdout.write("Loading...");
  const timer = setInterval(
    () => { process.stdout.write('.'); },
    1e3
  );
  try {
    return await task();
  } finally {
    clearInterval(timer);
    process.stdout.clearLine(0);
    process.stdout.cursorTo(0);
  }
}

which is then used like:

const { points, repositories } = await loadingIndicator(() => getRepositories(flags, filter));

What do you think?

KhatiaIvanova commented 2 years ago

It will work. We can update in that way.

KhatiaIvanova commented 2 years ago

Let me give some time @ljharb

abdumamdouh commented 2 years ago

Hello @ljharb , can you check now?

abdumamdouh commented 2 years ago

Looks pretty good!

@ljharb ready to be merged now?

ljharb commented 2 years ago

Yep! I'm rebasing it now, and will land it shortly.