Fix up sets logic #568
Fix up sets logic #568
Conversation
| module.exports = (cli, config, emitter) => { | ||
| const geminiPath = path.resolve(config.system.projectRoot, 'gemini'); |
There was a problem hiding this comment.
если у нас будут переданы файлы или сэты будут не пустыми, то этот путь высчитывается вхолостую. Лучше унести куда-нибудь в отдельную функцию и позвать при необходимости
3 similar comments
| return _.intersection(filesToFilter, this._files); | ||
| } | ||
|
|
||
| return this._files; |
There was a problem hiding this comment.
return _.isEmpty(files)
? this._files
: _.intersection(this._files, files);
There was a problem hiding this comment.
Или вот так:
if (_.isEmpty(filesToFilter)) {
filesToFilter = this._files;
}
return _.intersection(filesToFilter, this._files);
|
Посмотрел |
|
/ok только не забудь посквошить и удалить calibrate.min.js |
1 similar comment
|
🆗 |
| @@ -0,0 +1,9 @@ | |||
| 'use strict'; | |||
|
|
|||
| const FilesFilter = require('./index'); | |||
There was a problem hiding this comment.
можно просто require('./')
|
ИМХО, спорный момент, что это решение понятнее и проще 😄 но общий счет 2:1, поэтому 🆗 |
| if (!_.isEmpty(files)) { | ||
| this._set.files = _.intersection(files, this._set.files); | ||
| } | ||
| this._set.files = this._filesFilter.filter(files); |
There was a problem hiding this comment.
Эээ, ребят, вы серьезно?
3 класса, 1 из которых абстрактный вместо того чтобы сделать
if (isEmpty(this._set.files) {
return files;
} else if (isEmpty(files)) {
return this._set.files;
} else {
return _.intersection(files, this._set.files);
}
There was a problem hiding this comment.
Я бы добавил ещё фабрику для создания фильтров, чтобы не реквайрить 2 файла
There was a problem hiding this comment.
Даешь больше классов ;) и огнетушитель...
There was a problem hiding this comment.
здесь два разных поведения. Причем это отличие известно на этапе создания объекта:
- this._set.files - пустые, используем все переданные файлы
- this._set.files - не пустые, используем пересечение
Зачем оставлять это знание в рантайме? Почему бы не унести на этап создания объекта. Вас смущает, что объекты с одним методом или что?
There was a problem hiding this comment.
В плане рантайма это ничего экономит. filterFiles все равно зовется ровно один раз на каждый сет, что так, что так. В плане читаемости, вы правда уверены что эта череда классов удобнее чем 5 строчек, из которых явно видно что вернется или единственный непустой массив, или их пересечение?
There was a problem hiding this comment.
Нет, не уверены :) Но тут не череда классов, а еще один уровень абстракции (причем легонький).
И тут ничего не возвращается, а устанавливается. Так что код будет выглядеть как-то так:
if (isEmpty(this._set.files)) {
this._set.files = files;
} else if (!isEmpty(files)) {
this._set.files = _.intersection(files, this._set.files);
}Такая конструкция читается чуть сложнее: тут три ветки, цикломатическая сложность кода выше и сконцентрирована в одном методе.
Хотя, конечно, возможно и перемудрили
There was a problem hiding this comment.
Ну, ничего не межает логику вынести в приватный метод, а в filterFiles оставить только
this._set.files = this._getFilteredFiles(files);There was a problem hiding this comment.
а это правда будет проще, чем вариант с фильтрами?
| @@ -24,10 +27,22 @@ const loadSuites = (sets, emitter) => { | |||
| return rootSuite; | |||
| }; | |||
|
|
|||
| const filesExist = (configSets, cliPaths) => { | |||
There was a problem hiding this comment.
А почему вы тут присваиваете анонимные функции, присвоенные в переменную а не используете каноничное function ?
Fix for #569
@j0tunn, @sipayRT, @eGavr, @tormozz48, @DudaGod