Fixed obsessive deprecate warnings, added special util for modules and methods deprecation #313
Conversation
|
@blond посмотри пожалуйста |
| } | ||
| }; | ||
|
|
||
| exports.deprecateModule = function (module, removeVer, replacement) { |
There was a problem hiding this comment.
параметры лучше передать объектом, тогда в месте использования будет намного легче читать:
deprecate({module: 'test-logger', since: 'v1.0.0', replacedWith: 'mock-enb package'});ну и соответственно у тебя будет экспортиться одна функция, а method или module будет решаться по свойству
|
@SwinX отребейзь, пожалуйста, от мастера. |
|
Ремарка себе - на винде стек нужно чистить по-другому, конструкция путей отличается. |
…d methods deprecation
| var delayedMessages = []; | ||
| var isInitialized = false; | ||
|
|
||
| exports.initialize = function (mustShowWarnings) { |
There was a problem hiding this comment.
Я бы сделал вместо аргумента опцией opts.showWarnings.
There was a problem hiding this comment.
+1, это вообще распространенная практика - любые bool-аргументы делать опциями, тогда код читается лучше
|
@SwinX Не нужно сквошить коммиты пока идет ревью - так трудно понять что ты поправил, и приходится просматривать весь PR снова. |
| var module = message.module; | ||
| var since = message.since; | ||
| var replaceMethod = replaceMethod; | ||
| var replaceModule = replaceModule; |
|
Предлагаю посмотреть на какие-нибудь существующие модули, например вот: https://github.com/dougwilson/nodejs-depd |
|
|
||
| if (replaceModule) { | ||
| return message + colorize.yellow(replaceModule); | ||
| } |
There was a problem hiding this comment.
вот эти два if меняются на одну строчку:
return message + colorize.yellow(replaceMethod || replaceModule);There was a problem hiding this comment.
var msg = replaceMethod && replaceModule
? ...
: colorize.yellow(replaceMethod || replaceModule);
return 'Please use ' + msg;
@j0tunn, а смысл, если логирование нам нужно своё? |
| var deprecatePath = path.normalize(path.join(__dirname, '../../../lib/utils/deprecate.js')); | ||
|
|
||
| before(function () { | ||
| logStub = new sinon.stub(Logger.prototype, 'logWarningAction'); |
There was a problem hiding this comment.
лучше запользовать sandbox и стабать сразу весь Logger.prototype:
beforeEach(function () {
sandbox.stub(Logger.prototype);
});
afterEach(function () {
sandbox.restore();
});Ну и если запользовать chai.should(), то в тестах можно писать
Logger.prototype.logWarningAction.shoud.be.called;чуть длиннее получается, зато в тесте сразу видно, что будет вызвано в коде
| } | ||
|
|
||
| function buildRemoveVersionSentence(since) { | ||
| return since && since.length ? |
There was a problem hiding this comment.
зачем проверка since.length?
| console.log('`dir-glob` is deprecated!!!'); | ||
| console.log('It will be removed in v1.0.0!!!'); | ||
| console.log('Use `glob` package!!!'); | ||
| require('../utils/deprecate')({module: 'dir-glob', since: 'v1.0.0', replaceModule: 'glob'}); |
There was a problem hiding this comment.
Лучше не экономить на переменной. По мне читается сложновато.
|
👍 |
|
🆗 |
Fixed obsessive deprecate warnings, added special util for modules and methods deprecation
Resolves #303
Resolves #304
Resolves #305
Resolves #312