Skip to content
This repository has been archived by the owner on Feb 12, 2024. It is now read-only.

Progress bar for cli #994

Closed
wants to merge 8 commits into from
Closed

Conversation

bmordan
Copy link
Contributor

@bmordan bmordan commented Sep 5, 2017

go-ipfs

js-ipfs

This is the progress bar from https://github.com/visionmedia/node-progress which is being called in js-ipfs-unixfs-engine in the builder.

I have also done a little house keeping in the src/files/add as the callbacks were getting a little unwieldy.

let list = []
let currentBytes = 0

async.waterfall([
Copy link
Member

Choose a reason for hiding this comment

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

lets stick with const waterfall = require('async/waterfall) please

My PR failed to pass the Travis build because of examples/traverse-ipld-graphs/git.js I have had to add this file and correct the code style errors. This should make the tests pass for this PR.
Copy link
Member

@daviddias daviddias left a comment

Choose a reason for hiding this comment

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

LReallyGTM! @bmordan Could you confirm that it also works for folders?

@daviddias
Copy link
Member

Also, does this work over ipfs-api? (i.e with a daemon on?)

Copy link
Member

@daviddias daviddias left a comment

Choose a reason for hiding this comment

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

This PR needs:

  • work with the daemon on (though js-ipfs-api)
  • tests

@bmordan
Copy link
Contributor Author

bmordan commented Sep 7, 2017

@daviddias daviddias mentioned this pull request Sep 8, 2017
@bmordan
Copy link
Contributor Author

bmordan commented Sep 8, 2017

@dignifiedquire @diasdavid

Testing the progress bar

There is now:

  • a unit test for the progress bar in js-ipfs.
  • tests in js-ipfs-unixfs-engine
    to test that a function passed with options.progress to the Importer is called with the right payload
  • tests in js-ipfs-api to test that the progress function is called with the daemon running

What I tried to do yesterday was hook into progress.stdout and verify that the progress bar was being printed to the terminal.
I did manage this - is very sketchy and I decided to not submit it.

When the tests run progress.stdout is passed to the progress bar library but stdout.isTTY (is node a text terminal) is false so the library returns early and doesn't output anything.

To get the progress bar to output in tests means you have to highjack stdout in the tests and stub out the following:

  • isTTY = true
  • columns = 80
  • cursorTo = function () {}
  • clearLine = function () {}

Then the progress bar is piped to out most of the files add tests break because what is written to stdout has changed. Before starting to re-created a clearLine function to conditionally clear the progress bar once it is complete one has to stop and ask one's self. What am I doing here? This is a bit of a rabbit hole and I think the tests we have for the progress bar are sufficient.

However I can see the testing coverage has now fallen 😞

@daviddias daviddias added status/ready Ready to be worked P0 Critical: Tackled by core team ASAP and removed status/ready Ready to be worked labels Oct 13, 2017
@daviddias
Copy link
Member

Now done through #1036

@daviddias daviddias closed this Oct 22, 2017
@ghost ghost removed the status/ready Ready to be worked label Oct 22, 2017
MicrowaveDev pushed a commit to galtproject/js-ipfs that referenced this pull request May 22, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
P0 Critical: Tackled by core team ASAP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants