openzeppelin-sdk icon indicating copy to clipboard operation
openzeppelin-sdk copied to clipboard

Type and review Loggy's interface

Open spalladino opened this issue 5 years ago • 2 comments

Add typings for Loggy to avoid type errors, and review its interface (eg consider renaming noSpin to something clearer).

spalladino avatar Dec 11 '19 19:12 spalladino

I've implemented this in a branch. Will open a PR soon.

frangio avatar Dec 13 '19 14:12 frangio

One of the main things we should fix is the reference argument. It shouldn't be necessary to manually specify a string reference id, at least not in the way we use it, which is to later update the spinner state. We should automatically return a value from the log function that can be stored in a variable (only if necessary!) that either is an object with methods to update the spinner, or that can be passed as an argument to the log functions. This avoids the potential for accidental id reuse, and removes the obligation to define a reference when one isn't needed.

I also think we should remove the filename and function name arguments, and instead use Node.js's (i.e., V8's) stack traces to obtain that automatically.

Also, as mentioned in https://github.com/OpenZeppelin/openzeppelin-sdk/pull/1349#issuecomment-567067269, logging functions should simply be in a module, rather than in an object (or a class as originally proposed in #1349), and for testing purposes we should stub the output stream rather than the logging functions.

frangio avatar Dec 18 '19 15:12 frangio