#11 Add tests for i18n-lang-js technology

tormozz48 commented 9 years ago

Please review it

@blond @sipayRT

blond commented 9 years ago

Нужно добавить тест на случай, когда файл с кейсетами пустой.

tormozz48 commented 9 years ago

"Нужно добавить тест на случай, когда файл с кейсетами пустой."

А такой кейс реальный?
Пустой - это совсем пустой или module.exports = {} ?
Что мы должны получить в таком случае?

@blond

blond commented 9 years ago

А такой кейс реальный?

Да

Пустой - это совсем пустой или module.exports = {} ?

module.exports = {};

Что мы должны получить в таком случае?

Что-то в таком духе:

if (typeof BEM !== \'undefined\' && BEM.I18N) {
    BEM.I18N.lang(\'' + lang + '\');
}

tormozz48 commented 9 years ago

Я исправил все замечания по этому pr. Требуется вердикт

sipayRT commented 9 years ago

ну если тебе не по душе унести схему, то у меня других заметок нет. Кстати, посмотри, плз, мой комментарий - ты на него не ответил.

coveralls commented 9 years ago

Changes Unknown when pulling ce8c6922b577d0b51ab2b3751db2e5df95eeb505 on #11 into \ on master**.

tadatuta commented 9 years ago

кмк, пустой-пустой тоже может иметь смысл проверять, например, если файл с переводами закомментировали:

// module.exports = {
//  
// }

tormozz48 commented 9 years ago

"кмк, пустой-пустой тоже может иметь смысл проверять, например, если файл с переводами закомментировали:" - этот файл получается в результате работы предыдущей технологии i18n-merge-keysets.js и является автогенеренным. Сложно представить что его могули закомментировать

coveralls commented 9 years ago

Changes Unknown when pulling aa86f8d0ebfc9aded67e4e13dcf9e5e037d5f24e on #11 into \ on master**.

coveralls commented 9 years ago

Changes Unknown when pulling d6ba8aa12a08584f9c60c7265f1ba30eeb3c3104 on #11 into \ on master**.

sipayRT commented 9 years ago

@tormozz48 во, вот последний коммит прям красивый :) Вот такое же я бы сделал и для expected как-то так:

expected = [
    'if (typeof BEM !== \'undefined\' && BEM.I18N) {BEM.I18N.decl(\'scope\', {',
    '    "key": ' + (initial.mtime === modified.mtime) ? initial.val : modified.val,
    '}, {',
    '"lang": "lang"',
    '});',
    '',
    'BEM.I18N.lang(\'lang\');',
    '',
    '}',
    ''
].join('\n');

но если считаешь, что это нечитабельно, то можно забить

sipayRT commented 9 years ago

в общем, у меня всё :)

tormozz48 commented 9 years ago

@sipayRT Насчет expected есть 2 возражения:

Во-первых он и так с трудом читаемый
Любые дополнительные абстракции усложняют понимание

sipayRT commented 9 years ago

@tormozz48 вот я об этом и говорю - не понятно что в этом большом куске кода должно поменяться. А переменные хоть подсветятся :) Но я не настаиваю, можно и так оставить

coveralls commented 9 years ago

Changes Unknown when pulling 132e6af60c22eb78f4c39b1c4945e86d576ab157 on #11 into \ on master**.

enb / enb-bem-i18n

#11 Add tests for i18n-lang-js technology #18