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

Migrate CLI output to Logger #495

Closed
victorb opened this issue Sep 15, 2016 · 14 comments
Closed

Migrate CLI output to Logger #495

victorb opened this issue Sep 15, 2016 · 14 comments
Labels
exp/novice Someone with a little familiarity can pick up help wanted Seeking public contribution on this issue

Comments

@victorb
Copy link
Member

victorb commented Sep 15, 2016

With the new files APIs, I've started on a logger that has many benefits compared to the current approach of using console.log for output.

  • Intent is clear, console.log is often used to debugging, may not be clear it's actual output from CLI
  • Easier to turn on/off if we want to add a --quiet flag that only outputs some things
  • Abstract away the logging from the CLI itself
  • Easier to build things that continues on the same line, commands that takes time to execute.

I don't think this is a urgent refactor we have to do right now, but maybe we can start thinking about it a little, and write new code with the logger, rather than migrating everything at once.

@daviddias
Copy link
Member

👍 for having the ability to turn on/off.

Since we use debug for dev logs (log and log.err), let's call the 'logger' that prints info to the user some other thing. Maybe 'printer'?

@dignifiedquire
Copy link
Member

printer sounds good, and the method should be called print :)

@tarunbatra
Copy link
Contributor

or maybe different methods, like info, error to log messages of corresponding type.

@victorb
Copy link
Member Author

victorb commented Sep 24, 2016

@dignifiedquire agree that it should just be print, probably no need for many functions, just one called print and that would be about it.

@tarunbatra this is about printing output to the console and not logging things that can be controlled by for example log-level and such.

@daviddias
Copy link
Member

@tarunbatra for log messages we use debug

@tarunbatra
Copy link
Contributor

@diasdavid @victorbjelkholm thanx for clarifying! I was thinking otherwise.

@daviddias daviddias added the help wanted Seeking public contribution on this issue label Nov 12, 2016
@daviddias
Copy link
Member

@victorbjelkholm could you finish this since you got it started? :)

@victorb
Copy link
Member Author

victorb commented Nov 22, 2016

@diasdavid I just started using it lightly. I think we shouldn't migrate everything at once but rather think about it when writing new/refactoring code (and when reviewing) so at one point, we'll be using it 100%. Does that make sense for you?

@daviddias
Copy link
Member

Not really, it will confuse contributors and even ourselves at times as it will be always half done.

@victorb
Copy link
Member Author

victorb commented Nov 22, 2016

@diasdavid hm, ok. Will have to push this forward then, won't have any bandwidth currently to work on it...

@daviddias daviddias added exp/novice Someone with a little familiarity can pick up status/ready Ready to be worked and removed js-ipfs-backlog labels Dec 2, 2016
@daviddias daviddias added status/deferred Conscious decision to pause or backlog and removed status/ready Ready to be worked labels Jan 29, 2017
@daviddias daviddias added status/ready Ready to be worked and removed status/deferred Conscious decision to pause or backlog labels Apr 5, 2017
@daviddias daviddias added status/deferred Conscious decision to pause or backlog and removed status/ready Ready to be worked labels Jun 20, 2017
@rasmuserik
Copy link
Contributor

Hi, I might take a look at this,
as an entry point to get to know
the js-ipfs source code.

Feel free to assign it to me ;)

@daviddias
Copy link
Member

@rasmuserik thank you! Let me know if you get questions along the way

rasmuserik added a commit to rasmuserik/js-ipfs that referenced this issue Jul 23, 2017
License: MIT
Signed-off-by: Rasmus Erik Voel Jensen <github-rasmuserik@solsort.dk>
daviddias pushed a commit that referenced this issue Jul 24, 2017
* refactor: CLI should use print instead of console.log #495

License: MIT
Signed-off-by: Rasmus Erik Voel Jensen <github-rasmuserik@solsort.dk>

* feat: add quiet/verbose CLI flag, - related to #931

License: MIT
Signed-off-by: Rasmus Erik Voel Jensen <github-rasmuserik@solsort.dk>

* add test, and remove verbosity levels

License: MIT
Signed-off-by: Rasmus Erik Voel Jensen <github-rasmuserik@solsort.dk>
@rasmuserik
Copy link
Contributor

Can this be closed now, or is anything else missing, after merge of #931 ?

@daviddias daviddias removed status/deferred Conscious decision to pause or backlog labels Jul 26, 2017
@daviddias
Copy link
Member

You have completed this! Thanks @rasmuserik 💖🍾

dryajov pushed a commit that referenced this issue Sep 1, 2017
* refactor: CLI should use print instead of console.log #495

License: MIT
Signed-off-by: Rasmus Erik Voel Jensen <github-rasmuserik@solsort.dk>

* feat: add quiet/verbose CLI flag, - related to #931

License: MIT
Signed-off-by: Rasmus Erik Voel Jensen <github-rasmuserik@solsort.dk>

* add test, and remove verbosity levels

License: MIT
Signed-off-by: Rasmus Erik Voel Jensen <github-rasmuserik@solsort.dk>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
exp/novice Someone with a little familiarity can pick up help wanted Seeking public contribution on this issue
Projects
None yet
Development

No branches or pull requests

5 participants